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(aws-ecs): add ScheduledEc2Task L3 construct #2336

Merged
merged 10 commits into from
May 23, 2019
Merged

Conversation

piradeepk
Copy link
Contributor

@piradeepk piradeepk commented Apr 19, 2019

Defined a new construct to enable creating a ec2 task to be run at a scheduled interval

Fixes #2352


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@piradeepk piradeepk requested review from SoManyHs and a team as code owners April 19, 2019 17:57
@piradeepk piradeepk changed the title **feat** defined a new construct to enable creating a ec2 task to be run at a scheduled interval feat: defined a new construct to enable creating a ec2 task to be run at a scheduled interval Apr 22, 2019
/**
* The schedule expression used to configure the event rule.
*/
readonly scheduleExpression?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected syntax for schedule Expression? In the README the example says 'rate(1 minute)', which seems not the most intuitive syntax.

Copy link
Contributor Author

@piradeepk piradeepk Apr 22, 2019

Choose a reason for hiding this comment

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

The schedulingExpressions are values that are provided by cloudwatch events: https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/ScheduledEvents.html

I'll update the comment to reflect this though.

packages/@aws-cdk/aws-ecs/lib/scheduled-ecs-task.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/test/integ.scheduled-ecs-task.ts Outdated Show resolved Hide resolved
import { ScheduledEc2Task } from '../lib';

export = {
"Can create a scheduled Ec2 Task without a service"(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The phrasing of this unit test is a bit confusing. Is this just saying that we expect this construct to be a wrapper for an ECS task rather than a service? Also are there other API on the construct we should be unit testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this just saying that we expect this construct to be a wrapper for an ECS task rather than a service? - Yes
Also are there other API on the construct we should be unit testing? - Fair point, adding more unit tests.

packages/@aws-cdk/aws-ecs/lib/scheduled-ecs-task.ts Outdated Show resolved Hide resolved
taskDefinition.addContainer('ScheduledTaskContainer', {
image: props.image,
memoryLimitMiB: props.memoryLimitMiB !== undefined ? props.memoryLimitMiB : 256,
logging: props.enableLogging ? this.createAWSLogDriver(this.node.id) : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

enableLogging might be undefined -- you need to set the default to be true somewhere.

packages/@aws-cdk/aws-ecs/test/integ.scheduled-ecs-task.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/test/integ.scheduled-ecs-task.ts Outdated Show resolved Hide resolved
@SoManyHs SoManyHs changed the title feat: defined a new construct to enable creating a ec2 task to be run at a scheduled interval feat(aws-ecs): add ScheduledEc2Task L3 construct Apr 22, 2019
@SoManyHs
Copy link
Contributor

To be clear, this is not possible in Fargate, correct?

@piradeepk
Copy link
Contributor Author

To be clear, this is not possible in Fargate, correct?

Correct, to use Fargate, I would need to add a network configuration as networkmode is awsvpc. This is not currently supported by Cloudformation.

@SoManyHs
Copy link
Contributor

To be clear, this is not possible in Fargate, correct?

Correct, to use Fargate, I would need to add a network configuration as networkmode is awsvpc. This is not currently supported by Cloudformation.

What exactly isn't supported by CFN? We currently allow for awsvpc network mode for both Fargate Services and regular EC2 Task Definitions.

@jogold
Copy link
Contributor

jogold commented Apr 22, 2019

@SoManyHs

What exactly isn't supported by CFN?

The NetworkConfiguration property is currently missing in CloudFormation and needed for Fargate (allows to specify security group and subnets). See the differences between Amazon CloudWatch Events Rule EcsParameters (CloudFormation) and EcsParameters (CloudWatch PutTargets).

A Fargate scheduled task would need a Custom Resource. It would be a great addition to the CDK (need this in my projects) and I'm willing to PR for this but I'd prefer a more generic solution using what I proposed in #1850.

@piradeepk
Copy link
Contributor Author

piradeepk commented Apr 22, 2019

To be clear, this is not possible in Fargate, correct?

Correct, to use Fargate, I would need to add a network configuration as networkmode is awsvpc. This is not currently supported by Cloudformation.

What exactly isn't supported by CFN? We currently allow for awsvpc network mode for both Fargate Services and regular EC2 Task Definitions.

CloudFormation currently only supports clusters, services and taskdefinitions: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/cfn-reference-ecs.html.

TaskDefinitions contain a network mode, and for EC2, we can use none, bridge or the many others, but in order to use awsvpc, you need to create a NetworkConfiguration, which is not supported in the TaskDefinition, but is supported in the Service.

For this construct, we're creating a scheduled task (that is standalone task, not backed with a service), and for Fargate to support this feature, we'd need to have a CloudFormation create the concept of a standalone Task (not Task Definition) that can take a NetworkConfiguration property.

It works for ECS because we're using bridge as the NetworkMode, which does not require a NetworkConfiguration to be present.

@SoManyHs
Copy link
Contributor

@pkandasamy91 Ah, right -- I was more referring to the CFN support on CWE mentioned by @jogold (thanks for the comment). Appreciate the clarification.

packages/@aws-cdk/aws-ecs/lib/index.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/scheduled-ecs-task.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/scheduled-ecs-task.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/scheduled-ecs-task.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/test/test.scheduled-ecs-task.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/test/integ.scheduled-ecs-task.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@SoManyHs SoManyHs left a comment

Choose a reason for hiding this comment

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

Generally looks good -- could you combine #2363 with this PR?

Copy link
Contributor

@SoManyHs SoManyHs left a comment

Choose a reason for hiding this comment

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

Style nit about spaces, otherwise LGTM!

packages/@aws-cdk/aws-ecs/README.md Outdated Show resolved Hide resolved
@piradeepk
Copy link
Contributor Author

Squashed all commits

@piradeepk
Copy link
Contributor Author

Unit tests have been updated to handle merged changes.

@rix0rrr
Copy link
Contributor

rix0rrr commented May 23, 2019

I notice you made these changes on the master branch in your repo. That might cause you some trouble when trying to resync with upstream master later on. Don't forget to git reset --hard :).

@rix0rrr rix0rrr merged commit b9cbb6a into aws:master May 23, 2019
This was referenced Dec 12, 2019
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.

L3 Construct for creating a scheduled task on AWS ECS
4 participants