feat(core): add ability to resolve mount targets using EFS API #392

jusiskin · 2021-04-16T21:34:45Z

Resolves #391

Taken from that issue:

The RFDK code to mount EFS file-systems on an instance assumes that the instance belongs to a VPC with Amazon-supplied DNS (see VPC DNS documentation). When a VPC is configured to disable the Amazon-supplied DNS resolver, then the DNS names used by both the EFS mount helper and the NFS mounting code cannot be resolved and the mount will fail with an error showing up in the cloud-init log file, such as:

[   32.413572] cloud-init[3739]: Failed to resolve "fs-a1b2c3d4.efs.us-west-2.amazonaws.com" - check that your file system ID is correct.

Solution

The DNS names in the error message above correspond to EFS mount targets. These provide the endpoints that serve the networked file-system traffic. EFS automatically adds DNS entries into your VPC's Amazon-supplied Route 53 resolver, but when this feature is disabled, those names cannot be resolved using DNS.

Fortunately, the IP address of each mount target can be obtained by calling the EFS DescribeMountTargets API.

Added an optional boolean flag when EFS mounting script that will perform this lookup and bake the IP address into the host machine's /etc/hosts file to bypass DNS lookups. Using this looks like:

TypeScript:

new MountableEfs(scope, {
  // ...
  resolveMountTargetDnsWithApi: true,
});

Python:

MountableEfs(scope,
  # ...
  resolve_mount_target_dns_with_api=True,
)

Testing

Modified the All-In-AWS-Infrastructure-Basic example, enabling the above flag. Connected to the mounting instances using SSM Session Manager and confirmed:

The /etc/hosts file contains an entry for the mount target:

10.0.71.79 fs-a1b2c1d2.efs.us-west-2.amazonaws.com # Added by RFDK

The EFS file-system driver establishes a connection to the specified IP address:

$ sudo netstat -alnp | grep 10.0.71.79
tcp        0      0 10.0.0.13:55102         10.0.71.79:2049         ESTABLISHED 2306/stunnel

Deployment succeeds
Am able to connect to the farm, submit a job, view successful render logs (requires proper EFS mounting on the RenderQueue)

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

packages/aws-rfdk/lib/core/lib/mountable-efs.ts

ddneilson · 2021-04-16T21:47:51Z

packages/aws-rfdk/lib/core/scripts/bash/mountEfs.sh

+if [[ $RESOLVE_MOUNTPOINT_IP_VIA_API == "true"  ]]
+then
+  # jq is used to query the JSON API response
+  sudo "${PACKAGE_MANAGER}" install -y jq


Should check whether jq is present, and only hit the package manager if it is not. Use-case is an isolated machine with no network connectivity to the package repository.

Good call! I'll add that check first

ddneilson · 2021-04-16T21:49:25Z

packages/aws-rfdk/lib/core/scripts/bash/mountEfs.sh

+
+  if [[ $MNT_TARGET_RESOURCE_ID == fs-* ]]; then
+    # Mounting without an access point
+    MOUNT_POINT_IP=$(aws efs describe-mount-targets \


Missing failure handling -- we're not guaranteed that there is a MountTarget in the same AZ as the instance. Route53 would resolve the DNS with a randomly chosen IP in that case.

Same in the fsap-* case

Given the concern you mentioned about cross-AZ data charges, do you think we should replicate this behavior? Or is it better to fail? I'm learning towards failing with a user-friendly warning over using a random mount target. That way anyone using this could avoid the cross-AZ costs and simply add the missing mount target.

The calling code has a catch-all error handler, but you're right - currently this will not fail gracefully if there's no matching mount target. I'll fix that - thanks!

My opinion... We want to make sure that the customer knows that there could be cross-AZ data charges, but not prevent them from deploying in a cross-AZ manner. Throwing an error for cross-AZ would make a cross-AZ mount impossible, and there may be valid use-cases for that.

I've added logic to fall-back to using any available mount target with a warning being output that should get logged to CloudWatch. Let me know what you think.

ddneilson · 2021-04-16T21:53:14Z

packages/aws-rfdk/lib/core/lib/mountable-efs.ts

    target.userData.addCommands(
      'TMPDIR=$(mktemp -d)',
      'pushd "$TMPDIR"',
      `unzip ${mountScript}`,
-      `bash ./mountEfs.sh ${this.props.filesystem.fileSystemId} ${mountDir} ${mountOptionsStr}`,
+      `bash ./mountEfs.sh ${this.props.filesystem.fileSystemId} ${mountDir} ${mountOptionsStr} ${resolveMountTargetDnsWithApi}`,


I suggest reversing the ordering of mount options & the new arg. Reason being that mount options could be empty, but the new arg will never be.

ddneilson · 2021-04-16T21:53:47Z

packages/aws-rfdk/lib/core/scripts/bash/mountEfs.sh


 set -xeu

 if test $# -lt 2
 then
-  echo "Usage: $0 <file system ID> <mount path> [<mount options>]"
+  echo "Usage: $0 FILE_SYSTEM_ID MOUNT_PATH [MOUNT_OPTIONS] [RESOLVE_MOUNT_POINT_USING_API]"


Makes sense to me to make the new arg required. Two optional positional args can lead to confusion.

- Make RESOLVE_MOUNT_POINT_USING_API argument required and move before optional mount opts - Add graceful error-handling when no matching mount target found for instance's availability zone

- userdata runs as root and some AMIs might not have sudo installed

packages/aws-rfdk/lib/core/scripts/bash/mountEfs.sh

ddneilson

One small nit, and then I think we're good.

packages/aws-rfdk/lib/core/scripts/bash/mountEfs.sh

Co-authored-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com>

feat(core): add ability to resolve mount targets using EFS API

daa2fd7

ddneilson requested changes Apr 16, 2021

View reviewed changes

jusiskin added 2 commits April 19, 2021 16:26

chore(core): apply MR feedback

c57df3f

- Make RESOLVE_MOUNT_POINT_USING_API argument required and move before optional mount opts - Add graceful error-handling when no matching mount target found for instance's availability zone

chore(core): remove sudo from mountEfs.sh

1eb5c9b

- userdata runs as root and some AMIs might not have sudo installed

horsmand self-requested a review April 19, 2021 19:17

horsmand reviewed Apr 19, 2021

View reviewed changes

packages/aws-rfdk/lib/core/scripts/bash/mountEfs.sh Show resolved Hide resolved

jusiskin added the contribution/core This is a PR that came from AWS. label Apr 19, 2021

chore(core): fallback to using any mount target with a warning

edd4d51

jusiskin requested review from ddneilson and horsmand April 19, 2021 22:59

ddneilson requested changes Apr 20, 2021

View reviewed changes

packages/aws-rfdk/lib/core/scripts/bash/mountEfs.sh Outdated Show resolved Hide resolved

horsmand previously approved these changes Apr 20, 2021

View reviewed changes

chore(core): fix validation for number of required args in mountEfs.sh

a7ed34f

Co-authored-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com>

jusiskin dismissed horsmand’s stale review via a7ed34f April 20, 2021 15:59

jusiskin requested review from ddneilson and horsmand April 20, 2021 16:06

ddneilson approved these changes Apr 20, 2021

View reviewed changes

horsmand approved these changes Apr 20, 2021

View reviewed changes

horsmand merged commit 726fa84 into aws:mainline Apr 20, 2021

jusiskin deleted the resolve_efs_mount_targets_via_api branch April 26, 2021 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add ability to resolve mount targets using EFS API #392

feat(core): add ability to resolve mount targets using EFS API #392

jusiskin commented Apr 16, 2021 •

edited

Loading

ddneilson Apr 16, 2021

jusiskin Apr 16, 2021

ddneilson Apr 16, 2021

jusiskin Apr 16, 2021

ddneilson Apr 19, 2021

jusiskin Apr 19, 2021

ddneilson Apr 16, 2021

jusiskin Apr 19, 2021

ddneilson Apr 16, 2021

jusiskin Apr 19, 2021

ddneilson left a comment

feat(core): add ability to resolve mount targets using EFS API #392

feat(core): add ability to resolve mount targets using EFS API #392

Conversation

jusiskin commented Apr 16, 2021 • edited Loading

Solution

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddneilson left a comment

Choose a reason for hiding this comment

jusiskin commented Apr 16, 2021 •

edited

Loading