Fix ECS rollback stage does not remove canary created tasks #4465

ductnn · 2023-07-05T14:18:37Z

What this PR does / why we need it:

Fix Cancel with rollback does not remove canary created tasks

Which issue(s) this PR fixes:

Fixes #4329

Does this PR introduce a user-facing change?:

Is this breaking change:
How to migrate (if breaking change):
How are users affected by this change: Fix ECS rollback stage does not remove ECS canary created tasks

Signed-off-by: ductnn <[email protected]>

khanhtc1202 · 2023-07-05T14:37:29Z

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

+	// Delete Canary taskSet
+	canaryTaskSet, ok := in.MetadataStore.Shared().Get(canaryTaskSetARNKeyName)
+	if !ok {
+		in.LogPersister.Errorf("Unable to restore CANARY task set to clean: Not found")
+		return false
+	}
+	if err = client.DeleteTaskSet(ctx, *service, canaryTaskSet); err != nil {
+		in.LogPersister.Errorf("Failed to remove unused previous PRIMARY taskSet %s: %v", canaryTaskSet, err)
+		return false
+	}


Looks like we tried to clean the canary stub by this part, so how about to reuse clean function?
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/executor/ecs/ecs.go#L349

Thanks! I'll reuse clean function 🙏

@khanhtc1202 I fixed it! reuse clean function, please review it 🙏

khanhtc1202 · 2023-07-05T14:58:14Z

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

+	// Reset routing
+	routingTrafficCfg := provider.RoutingTrafficConfig{
+		{
+			TargetGroupArn: *primaryTargetGroup.TargetGroupArn,
+			Weight:         100,
+		},
+		{
+			TargetGroupArn: *canaryTargetGroup.TargetGroupArn,
+			Weight:         0,
+		},
+	}
+
+	currListenerArns, err := client.GetListenerArns(ctx, *primaryTargetGroup)
+	if err != nil {
+		in.LogPersister.Errorf("Failed to get current active listener: %v", err)
+		return false
+	}
+
+	if err := client.ModifyListeners(ctx, currListenerArns, routingTrafficCfg); err != nil {
+		in.LogPersister.Errorf("Failed to routing traffic to CANARY variant: %v", err)
+		return false
+	}
+
+	// Delete Canary taskSet
+	canaryTaskSet, ok := in.MetadataStore.Shared().Get(canaryTaskSetARNKeyName)
+	if !ok {
+		in.LogPersister.Errorf("Unable to restore CANARY task set to clean: Not found")
+		return false
+	}
+	if err = client.DeleteTaskSet(ctx, *service, canaryTaskSet); err != nil {
+		in.LogPersister.Errorf("Failed to remove unused previous PRIMARY taskSet %s: %v", canaryTaskSet, err)
+		return false
+	}
+


I think it's safer to do this after L182 (after we finish updating the rollbacked taskset as the primary taskset of the current service and remove the previous primary taskset), since it's more sense to only routing the traffic after primary tasksets are well-prepared and ready to handle incoming requests. Wdyt?

You're right, it's a secure choice to proceed 🙏

@khanhtc1202 I fixed it! reuse clean function, please review it 🙏

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

Signed-off-by: ductnn <[email protected]>

khanhtc1202 · 2023-07-06T14:20:01Z

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

+
+	currListenerArns, err := client.GetListenerArns(ctx, *primaryTargetGroup)
+	if err != nil {
+		in.LogPersister.Errorf("Failed to get current active listener: %v", err)


Suggested change

in.LogPersister.Errorf("Failed to get current active listener: %v", err)

in.LogPersister.Errorf("Failed to get current active listeners: %v", err)

nits

khanhtc1202 · 2023-07-06T14:20:39Z

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

+	}
+
+	if err := client.ModifyListeners(ctx, currListenerArns, routingTrafficCfg); err != nil {
+		in.LogPersister.Errorf("Failed to routing traffic to CANARY variant: %v", err)


Suggested change

in.LogPersister.Errorf("Failed to routing traffic to CANARY variant: %v", err)

in.LogPersister.Errorf("Failed to routing traffic to PRIMARY variant: %v", err)

Signed-off-by: ductnn <[email protected]>

khanhtc1202

Great, thank you 🙌

* Fix cannot remove canary created task Signed-off-by: ductnn <[email protected]> * Fix reuse clean function Signed-off-by: ductnn <[email protected]> * Update logs message for ECS routing stage rollback Signed-off-by: ductnn <[email protected]> --------- Signed-off-by: ductnn <[email protected]>

* Fix cannot remove canary created task Signed-off-by: ductnn <[email protected]> * Fix reuse clean function Signed-off-by: ductnn <[email protected]> * Update logs message for ECS routing stage rollback Signed-off-by: ductnn <[email protected]> --------- Signed-off-by: ductnn <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]>

) * Update web deps (#4451) Signed-off-by: khanhtc1202 <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> * Update quickstart to support arm hardware(ex: M1) (#4457) * Add: arch selection for mysql image Update chart values and templates for mysql deployment Signed-off-by: gitbluf <[email protected]> * Update: MySql image selector function to be more descriptive Signed-off-by: gitbluf <[email protected]> * Remove: mysql image set on piped installation Signed-off-by: gitbluf <[email protected]> * Update method name Signed-off-by: gitbluf <[email protected]> --------- Signed-off-by: gitbluf <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> * Support AWS LoadBalancer with multi listeners (#4462) * Support AWS LoadBalancer with multi listeners Signed-off-by: khanhtc1202 <[email protected]> * Fix go array index Signed-off-by: khanhtc1202 <[email protected]> * Rename symbol Signed-off-by: khanhtc1202 <[email protected]> --------- Signed-off-by: khanhtc1202 <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> * Specify Version For Templating Tools (#4463) * specify version for templating tools Signed-off-by: Viet Anh Pham Nhu <[email protected]> * Handle Error Both Install Helm And Kustomize Signed-off-by: Viet Anh Pham Nhu <[email protected]> --------- Signed-off-by: Viet Anh Pham Nhu <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> * Output usage only when flag parsing fails #4381 (#4464) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> * Fix ECS rollback stage does not remove canary created tasks (#4465) * Fix cannot remove canary created task Signed-off-by: ductnn <[email protected]> * Fix reuse clean function Signed-off-by: ductnn <[email protected]> * Update logs message for ECS routing stage rollback Signed-off-by: ductnn <[email protected]> --------- Signed-off-by: ductnn <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> * Update logs message for ECS routing stage executor (#4466) Signed-off-by: khanhtc1202 <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> * fix: update deprecated apiVersion since gke removed (#4469) GoogleCloudPlatform/gke-managed-certs#58 Signed-off-by: hungran <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> * Skip update AppState when no log updates (#4482) Signed-off-by: Kenta Kozuka <[email protected]> --------- Signed-off-by: khanhtc1202 <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: gitbluf <[email protected]> Signed-off-by: Viet Anh Pham Nhu <[email protected]> Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: ductnn <[email protected]> Signed-off-by: hungran <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Co-authored-by: Marko Petrovic <[email protected]> Co-authored-by: Viet Anh Pham Nhu <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]> Co-authored-by: Duc Tran <[email protected]> Co-authored-by: Henry Vu <[email protected]> Co-authored-by: Kurochan <[email protected]>

Fix cannot remove canary created task

9466f78

Signed-off-by: ductnn <[email protected]>

ductnn requested review from nghialv, khanhtc1202 and kentakozuka as code owners July 5, 2023 14:18

github-actions bot added the area/go label Jul 5, 2023

khanhtc1202 reviewed Jul 5, 2023

View reviewed changes

pkg/app/piped/executor/ecs/rollback.go Show resolved Hide resolved

khanhtc1202 changed the title ~~Fix cannot remove canary created task~~ Fix ECS rollback stage does not remove canary created task Jul 6, 2023

khanhtc1202 changed the title ~~Fix ECS rollback stage does not remove canary created task~~ Fix ECS rollback stage does not remove canary created tasks Jul 6, 2023

Fix reuse clean function

40c0f29

Signed-off-by: ductnn <[email protected]>

khanhtc1202 reviewed Jul 6, 2023

View reviewed changes

Update logs message for ECS routing stage rollback

2262cd4

Signed-off-by: ductnn <[email protected]>

khanhtc1202 approved these changes Jul 6, 2023

View reviewed changes

khanhtc1202 merged commit 1c2e5cb into pipe-cd:master Jul 6, 2023

kentakozuka mentioned this pull request Jul 12, 2023

Cherry-pick #4451 #4457 #4462 #4463 #4464 #4465 #4466 #4469 #4482 #4485

Merged

github-actions bot mentioned this pull request Jul 12, 2023

Release v0.44.2 #4486

Merged

github-actions bot mentioned this pull request Aug 23, 2023

Release v0.45.0 #4556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ECS rollback stage does not remove canary created tasks #4465

Fix ECS rollback stage does not remove canary created tasks #4465

ductnn commented Jul 5, 2023 •

edited by khanhtc1202

Loading

khanhtc1202 Jul 5, 2023

ductnn Jul 6, 2023

ductnn Jul 6, 2023

khanhtc1202 Jul 5, 2023 •

edited

Loading

ductnn Jul 6, 2023

ductnn Jul 6, 2023

khanhtc1202 Jul 6, 2023

khanhtc1202 Jul 6, 2023

khanhtc1202 left a comment

	in.LogPersister.Errorf("Failed to get current active listener: %v", err)
	in.LogPersister.Errorf("Failed to get current active listeners: %v", err)

	in.LogPersister.Errorf("Failed to routing traffic to CANARY variant: %v", err)
	in.LogPersister.Errorf("Failed to routing traffic to PRIMARY variant: %v", err)

Fix ECS rollback stage does not remove canary created tasks #4465

Fix ECS rollback stage does not remove canary created tasks #4465

Conversation

ductnn commented Jul 5, 2023 • edited by khanhtc1202 Loading

khanhtc1202 Jul 5, 2023

Choose a reason for hiding this comment

ductnn Jul 6, 2023

Choose a reason for hiding this comment

ductnn Jul 6, 2023

Choose a reason for hiding this comment

khanhtc1202 Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

ductnn Jul 6, 2023

Choose a reason for hiding this comment

ductnn Jul 6, 2023

Choose a reason for hiding this comment

khanhtc1202 Jul 6, 2023

Choose a reason for hiding this comment

khanhtc1202 Jul 6, 2023

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

ductnn commented Jul 5, 2023 •

edited by khanhtc1202

Loading

khanhtc1202 Jul 5, 2023 •

edited

Loading