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

Nextflow ECS Service and ALB #48

Merged
merged 15 commits into from
Aug 25, 2021
Merged

Nextflow ECS Service and ALB #48

merged 15 commits into from
Aug 25, 2021

Conversation

tthyer
Copy link
Contributor

@tthyer tthyer commented Aug 24, 2021

Primarily:

  • Adds ECS Service
  • Adds application load balancer

Secondarily:

  • Removes nextflow-ecs-capacity stack in favor of ApplicationAutoscaling resources in service stack
  • Move security group out of ecs cluster stack to decouple aurora stack from ecs cluster stack
  • Adds a Route 53 record set
  • Parameterizes values for easier updates and sharing values between stacks

Related JIRA tickets: WORKFLOWS-37 and WORKFLOWS-38

@tthyer tthyer requested a review from a team as a code owner August 24, 2021 00:26
auth:
github:
allow-list:
- tess.thyer@gmail.com
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This list will be built out more later -- this configuration does not appear to be taking hold. Waiting to hear back from support about this.

content: |
ECS_CLUSTER=${EcsClusterName}
ECS_BACKEND_HOST=
ECS_IMAGE_PULL_BEHAVIOR=prefer-cached
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this is the setting I discovered that we need in ECS last year. Got rate-limited again while testing this infrastructure -- it was crashing, so kept trying to re-pull all the images.

@tthyer tthyer requested a review from a team August 24, 2021 00:48
Copy link
Contributor

@BrunoGrandePhD BrunoGrandePhD left a comment

Choose a reason for hiding this comment

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

I did my best reviewing this PR since there are a lot of resources that I haven't used or written CFN templates for. It looks good to me! 🚀

I only had a few questions for my own personal curiosity.

templates/nextflow-ecs-service.yaml Show resolved Hide resolved
EcsAlbSecurityGroupIngress:
Type: AWS::EC2::SecurityGroupIngress
Properties:
GroupId: !GetAtt EcsAlbSecurityGroup.GroupId
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own information, is there an advantage of using GetAtt over Ref in this case? I can see how it's more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an important difference b/tw Ref and GetAtt. Most CFN resources have some default value they return when you use the Ref function. For example, the security group documentation indicates that Ref returns the resource id rather than the group id. Many CFN resources allow one to access additional fields -- this is always through GetAtt. For a security group the additional fields are the GroupId and the VpcId.

See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-security-group.html#aws-properties-ec2-security-group-return-values-ref

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I was under the impression that in this case, the resource ID returned by Ref would be the security group ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in the case of security groups resource id == group id but can't recall.

templates/nextflow-ecs-service.yaml Show resolved Hide resolved
Copy link
Contributor

@zaro0508 zaro0508 left a comment

Choose a reason for hiding this comment

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

phew, that was a wopper!

templates/nextflow-ecs-security-group.yaml Outdated Show resolved Hide resolved
templates/nextflow-ecs-security-group.yaml Show resolved Hide resolved
config/develop/nextflow-ecs-cluster.yaml Show resolved Hide resolved
templates/nextflow-ecs-cluster.j2 Show resolved Hide resolved
templates/nextflow-ecs-cluster.j2 Show resolved Hide resolved
templates/nextflow-ecs-cluster.j2 Outdated Show resolved Hide resolved
templates/nextflow-ecs-task-definition.yaml Outdated Show resolved Hide resolved
templates/nextflow-ecs-task-definition.yaml Show resolved Hide resolved
templates/nextflow-ecs-task-definition.yaml Show resolved Hide resolved
@tthyer tthyer requested a review from zaro0508 August 25, 2021 16:38
@tthyer tthyer merged commit 0c12358 into main Aug 25, 2021
@tthyer tthyer deleted the WORKFLOWS-37/alb branch August 25, 2021 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants