Support Canary for ECS without ELB or target groups #4690

t-kikuc · 2023-11-28T19:02:38Z

What this PR does / why we need it:

This is the simplest way to realize canary for ECS without target groups.

This PR supports the below stages (almost the same as the ELB case):

CanaryRollout
PrimaryRollout
CanaryClean
Rollback

NOTE: The TrafficRouting stage and Blue/Green Deployment are not supported due to ECS Service Discovery's constraints.

Which issue(s) this PR fixes:

Part of #4616
( or fixes )

Does this PR introduce a user-facing change?: no (new feature)

How are users affected by this change: no
Is this breaking change: no
How to migrate (if breaking change): no

Signed-off-by: t-kikuc <[email protected]>

codecov · 2023-11-28T19:06:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e5d7b4b) 30.86% compared to head (584beef) 30.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4690      +/-   ##
==========================================
- Coverage   30.86%   30.85%   -0.02%     
==========================================
  Files         221      221              
  Lines       25975    25975              
==========================================
- Hits         8018     8014       -4     
- Misses      17307    17310       +3     
- Partials      650      651       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

t-kikuc · 2023-11-29T01:37:32Z

/review

github-actions · 2023-11-29T01:37:53Z

PR Analysis

Main theme

type: Enhancement
description: Adds support for different access types (Load Balancer and Service Discovery) in the ECS deploy and rollback stages.

PR summary

This pull request introduces support for different access types (Load Balancer and Service Discovery) in the ECS deploy and rollback stages. It modifies the deploy.go and rollback.go files to handle the different access types and provide appropriate error messages when an unsupported access type is used.

Type of PR

Enhancement

PR Feedback:

General suggestions

The changes in this PR look good overall. The addition of support for different access types in the ECS deploy and rollback stages is a valuable enhancement. The code changes are clear and easy to understand.

Code feedback

pkg/app/piped/executor/ecs/deploy.go:13

This comment should be updated to reflect the new code changes. Instead of saying "Enables rolling out PRIMARY variant", it should say "Enables rolling out variants based on the specified access type".

pkg/app/piped/executor/ecs/deploy.go:32

The error message in this line should be updated to reflect the new code changes. Instead of saying "Unsupported access type ELB", it should say "Unsupported access type in stage for ECS application", where <accessType> is the value of e.appCfg.Input.AccessType and <stageName> is the name of the stage.

pkg/app/piped/executor/ecs/rollback.go:96

The comment in this line should be updated to reflect the new code changes. Instead of saying "Even if primary and canary target groups are not found, we can continue to rollback. So the accessType does not matter here.", it should say "Even if target groups are not found, we can continue to rollback. So the accessType does not matter here."

Documentation

There is no need to update the documentation for this PR.

Security concerns

No, this PR does not introduce any security concerns.

Signed-off-by: t-kikuc <[email protected]>

…com/pipe-cd/pipecd into ecs-service-discovery-canary-simple

t-kikuc · 2023-11-29T06:23:37Z

@khanhtc1202
Hi, I tested and it worked well.

khanhtc1202 · 2023-12-01T10:40:47Z

pkg/app/piped/executor/ecs/rollback.go

+	// Even if target groups are not found, we can continue to rollback.
+	// So the accessType does not matter here.


Should not mention unchanged thing here, let's not add confusing comments.

khanhtc1202 · 2023-12-01T10:41:06Z

pkg/app/piped/executor/ecs/deploy.go

 		return model.StageStatus_STAGE_FAILURE
 	}

 	return model.StageStatus_STAGE_SUCCESS
 }

 func (e *deployExecutor) ensureTrafficRouting(ctx context.Context) model.StageStatus {
+	// Traffic Routing is not supported for ECS Service Discovery.


Suggested change

// Traffic Routing is not supported for ECS Service Discovery.

// Traffic Routing is not supported for other kinds than ELB.

khanhtc1202 · 2023-12-01T10:41:38Z

pkg/app/piped/executor/ecs/rollback.go

+	// Even if target groups are not found, we can continue to rollback.
+	// So the accessType does not matter here.


Suggested change

// Even if target groups are not found, we can continue to rollback.

// So the accessType does not matter here.

Signed-off-by: t-kikuc <[email protected]>

…com/pipe-cd/pipecd into ecs-service-discovery-canary-simple

t-kikuc · 2023-12-01T11:14:31Z

@khanhtc1202
Thank you for your comments, and I fixed them.

khanhtc1202

great, thank you 🫡

* Support ECS CanaryRollout without TargetGroups Signed-off-by: t-kikuc <[email protected]> * Support ECS PrimaryRollout without TargetGroups Signed-off-by: t-kikuc <[email protected]> * Refuse ECS TrafficRouting stage for ServiceDiscovery Signed-off-by: t-kikuc <[email protected]> * Add a comment for ECS Rollback for ServiceDiscovery Signed-off-by: t-kikuc <[email protected]> * Add comments Signed-off-by: t-kikuc <[email protected]> * Removed unnecessary comments Signed-off-by: t-kikuc <[email protected]> --------- Signed-off-by: t-kikuc <[email protected]> Co-authored-by: Khanh Tran <[email protected]>

* Support ECS CanaryRollout without TargetGroups Signed-off-by: t-kikuc <[email protected]> * Support ECS PrimaryRollout without TargetGroups Signed-off-by: t-kikuc <[email protected]> * Refuse ECS TrafficRouting stage for ServiceDiscovery Signed-off-by: t-kikuc <[email protected]> * Add a comment for ECS Rollback for ServiceDiscovery Signed-off-by: t-kikuc <[email protected]> * Add comments Signed-off-by: t-kikuc <[email protected]> * Removed unnecessary comments Signed-off-by: t-kikuc <[email protected]> --------- Signed-off-by: t-kikuc <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: sZma5a <[email protected]>

* Support ECS CanaryRollout without TargetGroups Signed-off-by: t-kikuc <[email protected]> * Support ECS PrimaryRollout without TargetGroups Signed-off-by: t-kikuc <[email protected]> * Refuse ECS TrafficRouting stage for ServiceDiscovery Signed-off-by: t-kikuc <[email protected]> * Add a comment for ECS Rollback for ServiceDiscovery Signed-off-by: t-kikuc <[email protected]> * Add comments Signed-off-by: t-kikuc <[email protected]> * Removed unnecessary comments Signed-off-by: t-kikuc <[email protected]> --------- Signed-off-by: t-kikuc <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: 鈴木優耀 <[email protected]>

t-kikuc added 4 commits November 29, 2023 03:31

Support ECS CanaryRollout without TargetGroups

6a0633f

Signed-off-by: t-kikuc <[email protected]>

Support ECS PrimaryRollout without TargetGroups

880ca28

Signed-off-by: t-kikuc <[email protected]>

Refuse ECS TrafficRouting stage for ServiceDiscovery

f071bfb

Signed-off-by: t-kikuc <[email protected]>

Add a comment for ECS Rollback for ServiceDiscovery

8f22ec0

Signed-off-by: t-kikuc <[email protected]>

t-kikuc requested review from nghialv, khanhtc1202, kentakozuka and ffjlabo as code owners November 28, 2023 19:02

github-actions bot added the area/go label Nov 28, 2023

t-kikuc changed the title ~~Support ECS canary deployment without target groups~~ Support Canary for ECS without ELB or target groups Nov 28, 2023

Merge branch 'master' into ecs-service-discovery-canary-simple

c86458a

t-kikuc mentioned this pull request Nov 29, 2023

Support progressive delivery for ECS apps using Service Discovery #4616

Closed

t-kikuc added 2 commits November 29, 2023 15:21

Add comments

43e3af3

Signed-off-by: t-kikuc <[email protected]>

Merge branch 'ecs-service-discovery-canary-simple' of https://github.…

872dfbe

…com/pipe-cd/pipecd into ecs-service-discovery-canary-simple

t-kikuc mentioned this pull request Nov 30, 2023

Add an example of ECS canary for Service Discovery #4691

Merged

khanhtc1202 reviewed Dec 1, 2023

View reviewed changes

khanhtc1202 and others added 3 commits December 1, 2023 17:41

Merge branch 'master' into ecs-service-discovery-canary-simple

1b0794c

Removed unnecessary comments

c1f15be

Signed-off-by: t-kikuc <[email protected]>

Merge branch 'ecs-service-discovery-canary-simple' of https://github.…

584beef

…com/pipe-cd/pipecd into ecs-service-discovery-canary-simple

khanhtc1202 approved these changes Dec 1, 2023

View reviewed changes

khanhtc1202 merged commit d12bf7d into master Dec 1, 2023
13 of 14 checks passed

khanhtc1202 deleted the ecs-service-discovery-canary-simple branch December 1, 2023 12:34

github-actions bot mentioned this pull request Feb 6, 2024

Release v0.46.0 #4775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Canary for ECS without ELB or target groups #4690

Support Canary for ECS without ELB or target groups #4690

t-kikuc commented Nov 28, 2023

codecov bot commented Nov 28, 2023 •

edited

Loading

t-kikuc commented Nov 29, 2023

github-actions bot commented Nov 29, 2023

t-kikuc commented Nov 29, 2023

khanhtc1202 Dec 1, 2023

khanhtc1202 Dec 1, 2023 •

edited

Loading

khanhtc1202 Dec 1, 2023

t-kikuc commented Dec 1, 2023

khanhtc1202 left a comment

		// Even if target groups are not found, we can continue to rollback.
		// So the accessType does not matter here.

	// Traffic Routing is not supported for ECS Service Discovery.
	// Traffic Routing is not supported for other kinds than ELB.

Support Canary for ECS without ELB or target groups #4690

Support Canary for ECS without ELB or target groups #4690

Conversation

t-kikuc commented Nov 28, 2023

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

t-kikuc commented Nov 29, 2023

github-actions bot commented Nov 29, 2023

PR Analysis

Main theme

PR summary

Type of PR

PR Feedback:

General suggestions

Code feedback

pkg/app/piped/executor/ecs/deploy.go:13

pkg/app/piped/executor/ecs/deploy.go:32

pkg/app/piped/executor/ecs/rollback.go:96

Documentation

Security concerns

t-kikuc commented Nov 29, 2023

khanhtc1202 Dec 1, 2023

Choose a reason for hiding this comment

khanhtc1202 Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

khanhtc1202 Dec 1, 2023

Choose a reason for hiding this comment

t-kikuc commented Dec 1, 2023

khanhtc1202 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2023 •

edited

Loading

khanhtc1202 Dec 1, 2023 •

edited

Loading