Skip to content

Support recreate for ECS tasks #4608

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.

Sign up for GitHub

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

Merged
khanhtc1202 merged 2 commits into from
Oct 10, 2023
Merged

Support recreate for ECS tasks #4608

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5486fa3
Support singleton ECS task
khanhtc1202 Oct 6, 2023
20cfb77
Rename singleton to recreate
khanhtc1202 Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/app/piped/executor/ecs/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ func (e *deployExecutor) ensureSync(ctx context.Context) model.StageStatus {
return model.StageStatus_STAGE_FAILURE
}

if !sync(ctx, &e.Input, e.platformProviderName, e.platformProviderCfg, taskDefinition, servicedefinition, primary) {
singleton := e.appCfg.QuickSync.Singleton
if !sync(ctx, &e.Input, e.platformProviderName, e.platformProviderCfg, singleton, taskDefinition, servicedefinition, primary) {
Copy link
Member

@nghialv nghialv Oct 6, 2023

Choose a reason for hiding this comment

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

Do we need to update for rollback too?

Copy link
Member Author

@khanhtc1202 khanhtc1202 Oct 6, 2023

Choose a reason for hiding this comment

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

I think this feature is only for the sync stage, not progressive, since we only have one task which is available at a time, isn't it?

Copy link
Member

@nghialv nghialv Oct 6, 2023

Choose a reason for hiding this comment

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

I've forgotten most of the details regarding the code.
At this point, my main concern is to ensure that the rollback process also keeps the singleton property.

Copy link
Member Author

@khanhtc1202 khanhtc1202 Oct 6, 2023

Choose a reason for hiding this comment

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

I got your point 👍 Just plan to update the rollback process logic by another PR, so we can forget about that now.

Copy link
Member

@nghialv nghialv Oct 9, 2023

Choose a reason for hiding this comment

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

Ok!

return model.StageStatus_STAGE_FAILURE
}

Expand Down
37 changes: 32 additions & 5 deletions pkg/app/piped/executor/ecs/ecs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ func applyServiceDefinition(ctx context.Context, cli provider.Client, serviceDef
if err := cli.TagResource(ctx, *service.ServiceArn, serviceDefinition.Tags); err != nil {
return nil, fmt.Errorf("failed to update tags of ECS service %s: %w", *serviceDefinition.ServiceName, err)
}
// Re-assign tags to service object because UpdateService API doesn't return tags.
service.Tags = serviceDefinition.Tags

} else {
service, err = cli.CreateService(ctx, serviceDefinition)
Expand Down Expand Up @@ -242,7 +244,7 @@ func createPrimaryTaskSet(ctx context.Context, client provider.Client, service t
return nil
}

func sync(ctx context.Context, in *executor.Input, platformProviderName string, platformProviderCfg *config.PlatformProviderECSConfig, taskDefinition types.TaskDefinition, serviceDefinition types.Service, targetGroup *types.LoadBalancer) bool {
func sync(ctx context.Context, in *executor.Input, platformProviderName string, platformProviderCfg *config.PlatformProviderECSConfig, singleton bool, taskDefinition types.TaskDefinition, serviceDefinition types.Service, targetGroup *types.LoadBalancer) bool {
client, err := provider.DefaultRegistry().Client(platformProviderName, platformProviderCfg, in.Logger)
if err != nil {
in.LogPersister.Errorf("Unable to create ECS client for the provider %s: %v", platformProviderName, err)
Expand All @@ -263,10 +265,35 @@ func sync(ctx context.Context, in *executor.Input, platformProviderName string,
return false
}

in.LogPersister.Infof("Start rolling out ECS task set")
if err := createPrimaryTaskSet(ctx, client, *service, *td, targetGroup); err != nil {
in.LogPersister.Errorf("Failed to rolling out ECS task set for service %s: %v", *serviceDefinition.ServiceName, err)
return false
if singleton {
cnt := service.DesiredCount
// Scale down the service tasks by set it to 0
in.LogPersister.Infof("Scale down ECS desired tasks count to 0")
service.DesiredCount = 0
if _, err = client.UpdateService(ctx, *service); err != nil {
Copy link
Member

@nghialv nghialv Oct 6, 2023

Choose a reason for hiding this comment

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

Does this function wait for the completion of all tasks before proceeding?
My guess is no. 🤔
Therefore, perhaps we should have a waiting mechanism to ensure that no tasks are still running.

Copy link
Member Author

@khanhtc1202 khanhtc1202 Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's not necessary 🤔 AFAIK, when the service desired count is set to 0, the termination process begins. ECS will send a stop sign to any running tasks of that service and stop traffic coming to the service (since it's marked as inactivity). The tasks can keep working with its on-flight processes/requests, but no new coming traffic is accepted. So, rather than implement a wait mechanism here, I guess it's better to implement proper handling in the application code side, wdyt?

No New Traffic: However, during this process, the tasks that are being stopped will not accept new traffic. Requests sent to the tasks will be rejected or routed to other healthy tasks if you have implemented a load balancer or service discovery mechanism.

Copy link
Member

@nghialv nghialv Oct 9, 2023

Choose a reason for hiding this comment

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

Okay, after reviewing the old code, I think this approach will work well.
Since the old task sets will have been deleted before the desired count value is set again.

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

Lines 237 to 243 in 20cfb77

// Remove old taskSets if existed.
for _, prevTaskSet := range prevTaskSets {
if err = client.DeleteTaskSet(ctx, *prevTaskSet); err != nil {
return err
}
}

in.LogPersister.Errorf("Failed to stop service tasks: %v", err)
return false
}

in.LogPersister.Infof("Start rolling out ECS task set")
if err := createPrimaryTaskSet(ctx, client, *service, *td, targetGroup); err != nil {
in.LogPersister.Errorf("Failed to rolling out ECS task set for service %s: %v", *serviceDefinition.ServiceName, err)
return false
}

// Scale up the service tasks count back to its desired.
in.LogPersister.Infof("Scale up ECS desired tasks count back to %d", cnt)
service.DesiredCount = cnt
if _, err = client.UpdateService(ctx, *service); err != nil {
in.LogPersister.Errorf("Failed to turning back service tasks: %v", err)
return false
}
} else {
in.LogPersister.Infof("Start rolling out ECS task set")
if err := createPrimaryTaskSet(ctx, client, *service, *td, targetGroup); err != nil {
in.LogPersister.Errorf("Failed to rolling out ECS task set for service %s: %v", *serviceDefinition.ServiceName, err)
return false
}
}

in.LogPersister.Infof("Wait service to reach stable state")
Expand Down
4 changes: 4 additions & 0 deletions pkg/config/application_ecs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ type ECSTargetGroups struct {

// ECSSyncStageOptions contains all configurable values for a ECS_SYNC stage.
type ECSSyncStageOptions struct {
// Whether to delete old tasksets before creating new ones or not.
// If this is set, the application may be unavailable for a short of time during the deployment.
// Default is false.
Singleton bool `json:"singleton"`
}

// ECSCanaryRolloutStageOptions contains all configurable values for a ECS_CANARY_ROLLOUT stage.
Expand Down