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.

Sign up for GitHub

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

Jump to bottom

feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization #28315

Merged
merged 26 commits into from
Dec 22, 2023
Merged

feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization #28315

merged 26 commits into from
Dec 22, 2023

Conversation

AnuragMohapatra
Copy link
Contributor

@AnuragMohapatra AnuragMohapatra commented Dec 9, 2023 •
edited by kaizencc
Loading

Added an optional parameter that defaults to false over the CPU-based scaling policy that conflicts with the queue visible message-based policy.

When disabled this will stop the race condition issue mentioned in #20706 by only allowing the scaling of the number of messages on the queue similar to the SQS-Lambda pattern.

Note: If this parameter is enabled then this bug will crop up again and the user has to handle the container termination manually.

Updated integration tests and unit tests are working.

Closes #20706 .

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

@aws-cdk-automation aws-cdk-automation requested a review from a team December 9, 2023 08:45
@github-actions github-actions bot added bug This issue is a bug. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Dec 9, 2023
aws-cdk-automation
aws-cdk-automation previously requested changes Dec 9, 2023
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@AnuragMohapatra AnuragMohapatra changed the title fix(aws-ecs-patterns): Adding property to allow consumer to disable Cpu based scaling fix(ecs-patterns): Adding property to allow consumer to disable Cpu based scaling Dec 9, 2023
@AnuragMohapatra
Copy link
Contributor Author

Exemption Request : There is no integration tests for the library

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 9, 2023
lpizzinidev
lpizzinidev suggested changes Dec 10, 2023
View reviewed changes
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 👍
I left a couple of comments for improvements.

Notes:

  • This PR should be a feat: feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization
  • You should add an integration test for the new feature (you can use this as a blueprint, see guidelines on how to create/update one)

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/README.md Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts Outdated Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 10, 2023
AnuragMohapatra and others added 3 commits December 15, 2023 13:26
@AnuragMohapatra
Fix PR review comments
918174d
@AnuragMohapatra
Merge pull request #1 from AnuragMohapatra/main
b4da8f6 
Main
@AnuragMohapatra
Merge pull request #2 from AnuragMohapatra/fix/issue-20706-allowtosto…
785ed03 
…pscalingstrategyracecondition

Fix/issue 20706 allowtostopscalingstrategyracecondition
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 15, 2023 13:33

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@AnuragMohapatra
Copy link
Contributor Author

@lpizzinidev All review comments have been addressed.

I have found the integration tests are broken at the moment and an issue has been raised - #28383

I fixed it temporarily to get the current integ running but it does not look promising for some test cases.

lpizzinidev
lpizzinidev suggested changes Dec 16, 2023
View reviewed changes
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks for the changes 👍
I left other notes for some cleanup.
Please note the comment I left before:

This PR should be a feat: feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization

packages/aws-cdk-lib/aws-ecs-patterns/README.md Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/README.md Outdated Show resolved Hide resolved
@AnuragMohapatra AnuragMohapatra changed the title fix(ecs-patterns): Adding property to allow consumer to disable Cpu based scaling feat: feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization Dec 17, 2023
@AnuragMohapatra
Copy link
Contributor Author

@lpizzinidev Thanks for the review and updated all requested changes.

@lpizzinidev
Copy link
Contributor

@AnuragMohapatra
Please remove the double feat from the title: feat(ecs-patterns) ...

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023
@AnuragMohapatra AnuragMohapatra changed the title feat: feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization Dec 19, 2023
kaizencc
kaizencc previously requested changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Thanks for your PR and integ test @AnuragMohapatra. I have a few comments, and also please update the PR description to reflect the fact that an integ test here exists!

packages/aws-cdk-lib/aws-ecs-patterns/README.md Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Show resolved Hide resolved
@kaizencc kaizencc removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Dec 19, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023
@mergify mergify bot dismissed kaizencc’s stale review December 20, 2023 10:31

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 20, 2023
kaizencc
kaizencc reviewed Dec 22, 2023
View reviewed changes
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Thanks @AnuragMohapatra

kaizencc
kaizencc approved these changes Dec 22, 2023
View reviewed changes
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Oops :)

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

Thank you for contributing! Your pull request will be updated from main 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: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: dd0b470
  • 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 3cb3e02 into aws:main Dec 22, 2023
9 checks passed
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@AnuragMohapatra AnuragMohapatra mentioned this pull request Dec 23, 2023
Closed
2 tasks
@AnuragMohapatra
Copy link
Contributor Author

@kaizencc I think this also closed the issue #23624 but I was not aware this was there when I was adding the target CPU utilization property.

@github-actions github-actions bot mentioned this pull request Jan 1, 2024
Closed
paulhcsun pushed a commit to paulhcsun/aws-cdk that referenced this pull request Jan 5, 2024
@AnuragMohapatra @paulhcsun
feat(ecs-patterns): support disabling CPU-based scaling and custom ta…
278446d 
…rget utilization (aws#28315)

Added an optional parameter that defaults to false over the CPU-based scaling policy that conflicts with the queue visible message-based policy.

When disabled this will stop the race condition issue mentioned in aws#20706 by only allowing the scaling of the number of messages on the queue similar to the SQS-Lambda pattern.

Note: If this parameter is enabled then this bug will crop up again and the user has to handle the container termination manually.

Updated integration tests and unit tests are working.

Closes aws#20706 .

----

*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
Reviewers

@lpizzinidev lpizzinidev lpizzinidev approved these changes

@kaizencc kaizencc kaizencc approved these changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Milestone
No milestone
5 participants
@AnuragMohapatra @lpizzinidev @aws-cdk-automation @kaizencc @mohapatraanurag