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): support availability zone rebalancing #32263

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

isker
Copy link
Contributor

@isker isker commented Nov 23, 2024

Issue # (if applicable)

Closes #32226

Reason for this change

New AZ rebalancing feature is not yet supported in L2 constructs.

Description of changes

Add availabilityZoneRebalancing to the props of FargateService and Ec2Service, and validate all the documented constraints on it being ENABLED.

Description of how you validated changes

Unit and integration tests.

Checklist


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

@github-actions github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Nov 23, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team November 23, 2024 17:29
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 23, 2024
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.52%. Comparing base (f2ade56) to head (4b7ed11).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #32263   +/-   ##
=======================================
  Coverage   81.52%   81.52%           
=======================================
  Files         222      222           
  Lines       13715    13715           
  Branches     2417     2417           
=======================================
  Hits        11181    11181           
  Misses       2254     2254           
  Partials      280      280           
Flag Coverage Δ
suite.unit 81.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 80.97% <ø> (ø)
packages/aws-cdk-lib/core 82.09% <ø> (ø)

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 23, 2024
@isker
Copy link
Contributor Author

isker commented Nov 23, 2024

Hi @pahud, I saw you added an ecs-patterns-v2 label to #32226, which presumably means you want changes to those patterns to use this new feature as well. I have not implemented that here, at least not yet.

I have not used the patterns myself before, and when I took a look at them, I wasn't sure if I should default this feature to be enabled in services created by those patterns. What do you think?

Copy link
Contributor

@badmintoncryer badmintoncryer left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I've made some comments.

Comment on lines +1134 to +1139
*
* @param loadBalancer [disable-awslint:ref-via-interface]
Copy link
Contributor

Choose a reason for hiding this comment

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

I might not see many cases of overriding functions from a base class in CDK.
Perhaps it’s more common to define an interface in a base-service.ts file and have each subclass implement it. Please consider discussing this with the maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

attachToClassicLB is already on an interface, ILoadBalancerTarget:

/**
* Interface that is going to be implemented by constructs that you can load balance to
*/
export interface ILoadBalancerTarget extends IConnectable {
/**
* Attach load-balanced target to a classic ELB
* @param loadBalancer [disable-awslint:ref-via-interface] The load balancer to attach the target to
*/
attachToClassicLB(loadBalancer: LoadBalancer): void;
}

export abstract class BaseService extends Resource
implements IBaseService, elbv2.IApplicationLoadBalancerTarget, elbv2.INetworkLoadBalancerTarget, elb.ILoadBalancerTarget {

IIRC (I wrote this ~6 weeks ago 🌞) I had to disable this lint in order to be able to call super from the overriding methods. I need to call super because it refers to this private getter:

private get defaultLoadBalancerTarget() {
return this.loadBalancerTarget({
containerName: this.taskDefinition.defaultContainer!.containerName,
});
}

packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-service.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 Jan 7, 2025
@aws-cdk-automation
Copy link
Collaborator

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@isker isker force-pushed the az-rebalancing branch 2 times, most recently from ea590d6 to 52b9a58 Compare January 8, 2025 04:07
Comment on lines +191 to +193
if (!Token.isUnresolved(props.maxHealthyPercent) && props.maxHealthyPercent === 100) {
throw new Error('AvailabilityZoneRebalancing.ENABLED requires maxHealthyPercent > 100');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that by defining availabilityZoneRebalancing in BaseServiceOptions, all validations for maxHealthyPercent and those in attachToClassicLB() could be described in the BaseService class.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BaseService is also extended by ExternalService, where AvailabilityZoneRebalancing is not a supported feature.

Copy link
Contributor

@badmintoncryer badmintoncryer left a comment

Choose a reason for hiding this comment

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

Thanks!

app.synth() in the integ test code is not necessary. Please remove those lines!

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 8, 2025
Add `availabilityZoneRebalancing` to the props of FargateService and
Ec2Service, and validate all the documented constraints on it being
`ENABLED`.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 4b7ed11
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ecs: support new availabilityZoneRebalancing on services
3 participants