-
Notifications
You must be signed in to change notification settings - Fork 153
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
Support recreate for ECS tasks #4608
Conversation
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4608 +/- ##
==========================================
- Coverage 30.00% 29.98% -0.02%
==========================================
Files 221 221
Lines 25955 25955
==========================================
- Hits 7787 7783 -4
- Misses 17519 17523 +4
Partials 649 649
☔ View full report in Codecov by Sentry. |
pkg/app/piped/executor/ecs/deploy.go
Outdated
@@ -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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
// 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 | |
} | |
} | |
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
nit: change |
* Support singleton ECS task Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Rename singleton to recreate Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> --------- Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
* Display MySQL user defined error in API Key UI (#4590) * Display MySQL user defined error in UI Signed-off-by: Kenta Kozuka <kenta.kozuka@gmail.com> * Fix Unexpected empty arrow function Signed-off-by: Kenta Kozuka <kenta.kozuka@gmail.com> * Add tests Signed-off-by: Kenta Kozuka <kenta.kozuka@gmail.com> * Run subtests in parallel Signed-off-by: Kenta Kozuka <kenta.kozuka@gmail.com> --------- Signed-off-by: Kenta Kozuka <kenta.kozuka@gmail.com> * Display MySQL user defined error of piped, command, and application (#4597) Signed-off-by: Kenta Kozuka <kenta.kozuka@gmail.com> * Add service tags as takset task on create (#4598) Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * [ECS] Fix remove all previous active tasksets on QuickSync (#4600) * Remove previous ACTIVE tasksets if present on quicksync Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Remove GetPrimaryTaskSet interface Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> --------- Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Fix unable to fetch ECS taskset tags (#4605) Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Support recreate for ECS tasks (#4608) * Support singleton ECS task Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Rename singleton to recreate Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> --------- Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> --------- Signed-off-by: Kenta Kozuka <kenta.kozuka@gmail.com> Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> Co-authored-by: Kenta Kozuka <kenta.kozuka@gmail.com>
* Support singleton ECS task Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Rename singleton to recreate Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> --------- Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> Signed-off-by: moko-poi <mokopoi44@gmail.com>
* Support singleton ECS task Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Rename singleton to recreate Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> --------- Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
What this PR does / why we need it:
Support recreate ECS task while deploying. In such case, we need to configure the application config file like this
Which issue(s) this PR fixes:
Part of #4609
Does this PR introduce a user-facing change?: