feat(ecs): support availability zone rebalancing #32263

isker · 2024-11-23T17:29:41Z

Issue # (if applicable)

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

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

codecov · 2024-11-23T17:42:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.83%. Comparing base (361c7d3) to head (ea688b9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32263   +/-   ##
=======================================
  Coverage   80.83%   80.83%           
=======================================
  Files         236      236           
  Lines       14253    14253           
  Branches     2490     2490           
=======================================
  Hits        11522    11522           
  Misses       2446     2446           
  Partials      285      285

Flag	Coverage Δ
suite.unit	`80.83% <ø> (ø)`

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

Components	Coverage Δ
packages/aws-cdk	`79.57% <ø> (ø)`
packages/aws-cdk-lib/core	`82.20% <ø> (ø)`

isker · 2024-11-23T18:06:15Z

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?

badmintoncryer

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

badmintoncryer · 2024-11-27T02:59:01Z

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

+   *
+   * @param loadBalancer [disable-awslint:ref-via-interface]


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.

attachToClassicLB is already on an interface, ILoadBalancerTarget:

aws-cdk/packages/aws-cdk-lib/aws-elasticloadbalancing/lib/load-balancer.ts

Lines 144 to 153 in 92d03e2

/**

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

}

aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

Lines 511 to 512 in 92d03e2

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:

aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

Lines 1454 to 1458 in 92d03e2

private get defaultLoadBalancerTarget() {

return this.loadBalancerTarget({

containerName: this.taskDefinition.defaultContainer!.containerName,

});

}

packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-service.ts

packages/aws-cdk-lib/aws-ecs/lib/fargate/fargate-service.ts

aws-cdk-automation · 2025-01-08T00:11:29Z

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.

badmintoncryer · 2025-01-08T13:26:10Z

packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-service.ts

+      if (!Token.isUnresolved(props.maxHealthyPercent) && props.maxHealthyPercent === 100) {
+        throw new Error('AvailabilityZoneRebalancing.ENABLED requires maxHealthyPercent > 100');
+      }


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?

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

badmintoncryer

Thanks!

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

...aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.availability-zone-rebalancing.ts

...cdk-testing/framework-integ/test/aws-ecs/test/fargate/integ.availability-zone-rebalancing.ts

GavinZZ · 2025-02-04T23:59:55Z

@isker can you fix the conflicts please? I'm happy to give this PR a review this week and try to get this merged.

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

isker · 2025-02-05T02:26:49Z

@GavinZZ done.

GavinZZ

LGTM, thank you!

mergify · 2025-02-05T18:41:32Z

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).

mergify · 2025-02-05T18:47:25Z

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 · 2025-02-05T19:31:40Z

AWS CodeBuild CI Report

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

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

mergify · 2025-02-05T19:32:05Z

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).

github-actions · 2025-02-05T19:32:23Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Nov 23, 2024

aws-cdk-automation requested a review from a team November 23, 2024 17:29

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

isker mentioned this pull request Nov 23, 2024

feat(ecs): add availabilityZoneRebalancing property to EcsService #32259

Closed

1 task

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

github-actions bot mentioned this pull request Dec 1, 2024

Monthly PR metrics report #32340

Closed

badmintoncryer suggested changes Jan 7, 2025

View reviewed changes

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

isker force-pushed the az-rebalancing branch 2 times, most recently from ea590d6 to 52b9a58 Compare January 8, 2025 04:07

badmintoncryer reviewed Jan 8, 2025

View reviewed changes

badmintoncryer approved these changes Jan 8, 2025

View reviewed changes

...aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.availability-zone-rebalancing.ts Outdated Show resolved Hide resolved

...cdk-testing/framework-integ/test/aws-ecs/test/fargate/integ.availability-zone-rebalancing.ts Outdated Show resolved Hide resolved

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 8, 2025

isker force-pushed the az-rebalancing branch 2 times, most recently from 3a1715c to 4b7ed11 Compare January 9, 2025 05:37

GavinZZ self-assigned this Feb 3, 2025

feat(ecs): support availability zone rebalancing

46a5184

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

isker force-pushed the az-rebalancing branch from 4b7ed11 to 46a5184 Compare February 5, 2025 01:54

GavinZZ approved these changes Feb 5, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 5, 2025

Merge branch 'main' into az-rebalancing

ea688b9

mergify bot merged commit a8e2622 into aws:main Feb 5, 2025
20 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs): support availability zone rebalancing #32263

feat(ecs): support availability zone rebalancing #32263

isker commented Nov 23, 2024

codecov bot commented Nov 23, 2024 •

edited

Loading

isker commented Nov 23, 2024

badmintoncryer left a comment

badmintoncryer Nov 27, 2024

isker Jan 7, 2025

aws-cdk-automation commented Jan 8, 2025

badmintoncryer Jan 8, 2025

isker Jan 8, 2025

badmintoncryer left a comment

GavinZZ commented Feb 4, 2025

isker commented Feb 5, 2025

GavinZZ left a comment

mergify bot commented Feb 5, 2025

mergify bot commented Feb 5, 2025

aws-cdk-automation commented Feb 5, 2025

mergify bot commented Feb 5, 2025

github-actions bot commented Feb 5, 2025

	/**
	* 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 {

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

feat(ecs): support availability zone rebalancing #32263

feat(ecs): support availability zone rebalancing #32263

Conversation

isker commented Nov 23, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

codecov bot commented Nov 23, 2024 • edited Loading

Codecov Report

isker commented Nov 23, 2024

badmintoncryer left a comment

Choose a reason for hiding this comment

badmintoncryer Nov 27, 2024

Choose a reason for hiding this comment

isker Jan 7, 2025

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 8, 2025

badmintoncryer Jan 8, 2025

Choose a reason for hiding this comment

isker Jan 8, 2025

Choose a reason for hiding this comment

badmintoncryer left a comment

Choose a reason for hiding this comment

GavinZZ commented Feb 4, 2025

isker commented Feb 5, 2025

GavinZZ left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 5, 2025

mergify bot commented Feb 5, 2025

aws-cdk-automation commented Feb 5, 2025

AWS CodeBuild CI Report

mergify bot commented Feb 5, 2025

github-actions bot commented Feb 5, 2025

codecov bot commented Nov 23, 2024 •

edited

Loading