fix(ecs): Allow scheduling DAEMON services even if no EC2 capacity attached to cluster (#25306) #25328
Conversation
|
|
github-actions
bot
added
the
valued-contributor
github-actions
bot
added
effort/medium
There was a problem hiding this comment.
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.
otterley
changed the title
otterley
force-pushed
the
fix-25306
branch
2 times, most recently
from
483aabd to
d55a338
Compare
|
Exemption Request - no integration test is needed here because this fix only addresses a compile-time validation issue. No changes to the rendered stack templates are made. |
aws-cdk-automation
added
the
pr-linter/exemption-requested
…tached to cluster (aws#25306) It can be useful to allow DAEMON services to be scheduled to a cluster even if there is no EC2 capacity attached to it yet, under the assumption that capacity will be attached to the cluster later. We no longer fail validation if this happens.
| @@ -249,7 +249,7 @@ export class Ec2Service extends BaseService implements IEc2Service { | |||
| */ | |||
| private validateEc2Service(): string[] { | |||
| const ret = new Array<string>(); | |||
| if (!this.cluster.hasEc2Capacity) { | |||
| if (!this.daemon && !this.cluster.hasEc2Capacity) { | |||
There was a problem hiding this comment.
I'm not sure I see how this is any different from the closed issue that is related. The ask is to be able to add services to a cluster that don't have any capacity. Sounds like you are proposing that you found a loophole in the original issue because we never specifically mentioned daemon services 😁
I think what we need instead is a new way of managing capacity in clusters. You need capacity in a cluster so we don't want to remove that check, but we should have a way to tell the cluster that you have capacity.
There was a problem hiding this comment.
@corymhall I completely understand your point of view. As I said in #25306 (comment) - I don't necessarily believe we should fail validation for lack of capacity. I purposely kept this PR small in scope because I think it's more easily defensible, whereas the larger question still needs to be resolved.
There was a problem hiding this comment.
@otterley I disagree with your comment. If we have reason to believe that a users configuration will fail once deployed we should fail synthesis.
The bottom line is that multiple CDK team members have said the type of change that we will support. This PR falls under the same use case.
|
@pahud are we good to merge? |
corymhall
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
It can be useful to allow DAEMON services to be scheduled to a cluster even if there is no EC2 capacity attached to it yet, under the assumption that capacity will be attached to the cluster later. We no longer fail validation if this happens.
Fixes #25306
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license