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(ecs-service-extensions): Target tracking policies for Service Extensions #17101

Merged
merged 9 commits into from Nov 6, 2021

Conversation

upparekh
Copy link
Contributor


This PR adds desiredCount, targetCpuUtilization and targetMemoryUtilization to the service construct. It also adds requestsPerTarget to the HttpLoadBalancerExtension props to allow adding target tracking policy based on the ALB request count.

It will be followed by another PR to configure queue auto scaling for the SQS Queues in the QueueExtension.

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

@gitpod-io
Copy link

gitpod-io bot commented Oct 21, 2021

@upparekh upparekh self-assigned this Oct 21, 2021
@github-actions github-actions bot added the @aws-cdk/aws-sqs Related to Amazon Simple Queue Service label Oct 21, 2021
@upparekh upparekh added @aws-cdk-containers/ecs-service-extensions Related to ecs-service-extensions package and removed @aws-cdk/aws-sqs Related to Amazon Simple Queue Service labels Oct 21, 2021
@njlynch njlynch removed their assignment Oct 25, 2021
});

// THEN
expect(stack).toHaveResourceLike('AWS::ApplicationAutoScaling::ScalingPolicy', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow print that maxTaskCount is 5 here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed probably can't tell from this resource but the other ones like ecs service

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I meant to say can we have an expect statement to print that maxTaskCount is 5

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm the unit test is usually meant to check if the CFN template we generate is expected, instead of if the construct itself. maxTaskCount is a field of the construct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I didn't include a check for maxCapacity of the ScalableTarget here because we configure it in the service construct but it makes sense to add a check for it here as well! Will update!

new Service(stack, 'my-service', {
environment,
serviceDescription,
autoScaleTaskCount: {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we send autoScaleTaskCount: {}

Copy link
Contributor

Choose a reason for hiding this comment

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

maxTaskCount is a required field. If autoScaleTaskCount is undefined then we error out i guess? https://github.com/aws/aws-cdk/pull/17101/files#diff-77a32c641e0209aa8836c64a4f58132a141ad27eab31be695d247e8f4015a8a0R135

Copy link
Contributor

Choose a reason for hiding this comment

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

But does it consider {} as undefined is the question

Copy link
Contributor

Choose a reason for hiding this comment

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

{} is not undefined and it will be invalid input and not pass the static check i think

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that makes sense 👍🏼

Copy link
Contributor

@paragbhingre paragbhingre left a comment

Choose a reason for hiding this comment

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

Looks great to me, just have couple of questions. 🎉

@mergify
Copy link
Contributor

mergify bot commented Nov 5, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: d953753
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 6420b18 into aws:master Nov 6, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 6, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

iliapolo pushed a commit that referenced this pull request Nov 7, 2021
…tensions (#17101)

----
This PR adds `desiredCount`, `targetCpuUtilization` and `targetMemoryUtilization` to the service construct. It also adds `requestsPerTarget` to the `HttpLoadBalancerExtension` props to allow adding target tracking policy based on the ALB request count.

It will be followed by another PR to configure queue auto scaling for the SQS Queues in the `QueueExtension`.

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@upparekh upparekh deleted the upparekh/auto-scaling-extensions branch November 9, 2021 01:11
mergify bot pushed a commit that referenced this pull request Jan 12, 2022
…ension (#17802)

----
This PR deprecates one of the existing `scaleOnCpuUtilization` extension. We recommend users to configure task auto scaling using the [`autoScaleTaskCount`](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk-containers/ecs-service-extensions/lib/service.ts#L61) in the `Service` construct. 

Related PR: #17101 

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…tensions (aws#17101)

----
This PR adds `desiredCount`, `targetCpuUtilization` and `targetMemoryUtilization` to the service construct. It also adds `requestsPerTarget` to the `HttpLoadBalancerExtension` props to allow adding target tracking policy based on the ALB request count.

It will be followed by another PR to configure queue auto scaling for the SQS Queues in the `QueueExtension`.

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…ension (aws#17802)

----
This PR deprecates one of the existing `scaleOnCpuUtilization` extension. We recommend users to configure task auto scaling using the [`autoScaleTaskCount`](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk-containers/ecs-service-extensions/lib/service.ts#L61) in the `Service` construct. 

Related PR: aws#17101 

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk-containers/ecs-service-extensions Related to ecs-service-extensions package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants