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

feat(examples): local zones examples #314

Merged
merged 10 commits into from
Jun 22, 2021
Merged

Conversation

horsmand
Copy link
Contributor

Fixes #313


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

@jusiskin jusiskin added the contribution/core This is a PR that came from AWS. label Feb 10, 2021
@horsmand horsmand changed the title Local Zones Example feat(examples): local zones examples Feb 11, 2021
examples/deadline/Local-Zone/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/.gitignore Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/ts/.gitignore Outdated Show resolved Hide resolved
# to pin to. Some examples of pinned version values are "10", "10.1", or "10.1.13"
self.deadline_version: Optional[str] = '10.1.13.1'

# A map of regions to Deadline Client Linux AMIs.As an example, the Linux Deadline 10.1.13.1 AMI ID
Copy link

Choose a reason for hiding this comment

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

Nit: Missing space between period and 'As'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


from aws_cdk.core import (
Stack,
Construct
Copy link

Choose a reason for hiding this comment

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

Nit: The imports aren't alphabetical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

examples/deadline/Local-Zone/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
# It is being deployed into the standard availability zones, but has access to the worker
# instances that get deployed into a local zone. Not a critical component of the farm, so
# this can be safely removed.
self.bastion = BastionHostLinux(
Copy link

Choose a reason for hiding this comment

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

Mentioned elsewhere, but mentioning here: we should always recommend using SSM over Bastions (where possible!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@horsmand horsmand requested a review from ddneilson February 17, 2021 17:37
Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

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

Just some small comments, otherwise this looks good to me.

examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/ts/bin/app.ts Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/ts/lib/compute-tier.ts Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/ts/lib/compute-tier.ts Outdated Show resolved Hide resolved
jericht
jericht previously approved these changes Mar 8, 2021
jericht
jericht previously approved these changes Mar 10, 2021
jericht
jericht previously approved these changes Apr 8, 2021
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Great job! Just a few small proposed changes but otherwise this is solid

examples/deadline/Local-Zone/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
availabilityZones: config.availabilityZonesLocal,
renderQueue: service.renderQueue,
workerMachineImage: MachineImage.genericLinux(config.deadlineClientLinuxAmiMap),
keyPairName: config.keyPairName ? config.keyPairName : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can this be simply:

keyPairName: config.keyPairName,

@@ -0,0 +1,39 @@
# RFDK Sample Application - Local Zones
Copy link
Contributor

@jusiskin jusiskin Apr 9, 2021

Choose a reason for hiding this comment

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

This example does a great job of showing how to deploy the Workers to a local zone - great work!

I think one thing that might be missing though is an explanation of why this is important/useful. RFDK users want this so they can have a low-latency connection between their AWS infrastructure and their infrastructure hosted outside AWS. It might help to spend some time at the beginning of this README setting the stage here. It may also help to add some guidance, links, and next steps to point users in the right direction for how they'd take this example and proceed to the next step of connecting their infrastructure outside of AWS.

One good resource we could link to is our developer guide documentation on connecting to a RFDK render farm

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding documentation for this in the top-level README.

As discussed offline, let's add a "next steps" section to the Python/TS READMEs that list the steps required to connect an on-premise networked file-system to the Workers. Even if we don't have all of the steps completely detailed, for now having a checklist would help guide readers to research and complete them. These are the steps that I foresee here:

  1. Follow the connecting to your render farm dev guide page to modify the NetworkTier with the the VPN connection.
  2. Ensure there is a route from the worker subnet to the on-prem file-system
  3. Ensure the VPN endpoint's security group has ingress rules to allow NFS port traffic from the Workers
  4. Add user-data to mount the NFS on the compute tier
  5. (optional) set up path mapping rules in Deadline

jericht
jericht previously approved these changes Jun 10, 2021
jusiskin
jusiskin previously approved these changes Jun 22, 2021
Copy link
Contributor

@jusiskin jusiskin 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. Thanks for all your hard work on this one!

Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

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

Just a few nits/typos, otherwise this looks great!

examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/python/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/ts/README.md Outdated Show resolved Hide resolved
examples/deadline/Local-Zone/ts/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jericht jericht 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, thanks!

@jusiskin jusiskin dismissed ddneilson’s stale review June 22, 2021 19:46

All feedback was addressed

@jusiskin jusiskin merged commit 1fe72a0 into aws:mainline Jun 22, 2021
@horsmand horsmand deleted the local-zones branch June 22, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local Zones Example
5 participants