Skip to content
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

feat: add detailed timeout options #81

Merged
merged 9 commits into from
Jun 27, 2024
Merged

feat: add detailed timeout options #81

merged 9 commits into from
Jun 27, 2024

Conversation

keroxp
Copy link
Member

@keroxp keroxp commented Jun 25, 2024

  • Added detailed options for all ECS related commands. Previously those were fixed as 300s.
--serviceStableTimeout value    max duration seconds for waiting service stable (default: 900) [$CAGE_SERVICE_STABLE_TIMEOUT]
--taskHealthCheckTimeout value  max duration seconds for waiting canary task health check (default: 900) [$CAGE_TASK_HEALTH_CHECK_TIMEOUT]
--taskRunningTimeout value      max duration seconds for waiting canary task running (default: 900) [$CAGE_TASK_RUNNING_TIMEOUT]
--taskStoppedTimeout value      max duration seconds for waiting canary task stopped (default: 900) [$CAGE_TASK_STOPPED_TIMEOUT]
  • Changed rollout cmd to use deregistration_delay.timeout_seconds by fetching from ALB target group's attributes if exists (Default: 300s).
  • Changed rollout cmd not to stop to stoup canary task even if any error occurs during deregistering canary task from target group.

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 81.60000% with 23 lines in your changes missing coverage. Please review.

Project coverage is 74.36%. Comparing base (27d9a83) to head (595a59d).

Files Patch % Lines
canary_task.go 60.00% 8 Missing and 10 partials ⚠️
cli/cage/commands/command.go 0.00% 3 Missing ⚠️
rollout.go 0.00% 0 Missing and 1 partial ⚠️
up.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   73.21%   74.36%   +1.15%     
==========================================
  Files          17       18       +1     
  Lines         896      987      +91     
==========================================
+ Hits          656      734      +78     
- Misses        139      145       +6     
- Partials      101      108       +7     

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

@keroxp keroxp requested a review from alfnets June 25, 2024 11:52
@keroxp keroxp marked this pull request as ready for review June 25, 2024 11:52
@keroxp keroxp requested a review from takkyuuplayer June 26, 2024 06:13
@keroxp
Copy link
Member Author

keroxp commented Jun 26, 2024

Diffs from merge are confirmed.

index 1234ab8..48b7e12 100644
--- a/cage.go
+++ b/cage.go
@@ -43,7 +43,7 @@ func NewCage(input *Input) Cage {
        return &cage{
                Input: input,
                Timeout: timeout.NewManager(
-                       10*time.Minute,
+                       15*time.Minute,
                        &timeout.Input{
                                TaskRunningWait:     taskRunningWait,
                                TaskHealthCheckWait: taskHealthCheckWait,

Copy link
Contributor

@takkyuuplayer takkyuuplayer left a comment

Choose a reason for hiding this comment

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

I'll check after #84 is merged into main.

Copy link

@alfnets alfnets left a comment

Choose a reason for hiding this comment

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

I've made a few comments, but overall it looks good.

Copy link

Choose a reason for hiding this comment

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

[NITS] [IMO] We might not need to include .vscode/settings.json in the git repository.

Copy link
Member Author

@keroxp keroxp Jun 27, 2024

Choose a reason for hiding this comment

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

No. We need to include settings. Sharing settings is quite common way in modern JS development, as there're essential settings in addition to for each personal environment. Those should be considered, but useful in most case.

https://github.com/vitest-dev/vitest/tree/main/.vscode
https://github.com/jestjs/jest/tree/main/.vscode

cli/cage/commands/flags.go Outdated Show resolved Hide resolved
timeout/timeout_test.go Outdated Show resolved Hide resolved
@keroxp keroxp requested a review from alfnets June 27, 2024 05:34
Copy link

@alfnets alfnets left a comment

Choose a reason for hiding this comment

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

LGTM!

@takkyuuplayer takkyuuplayer self-requested a review June 27, 2024 05:45
run_test.go Outdated Show resolved Hide resolved
@@ -306,10 +310,36 @@ func (c *CanaryTask) waitUntilTargetHealthy(
}
}

func (c *CanaryTask) targetDeregistrationDelay(ctx context.Context) (time.Duration, error) {
deregistrationDelay := 300 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

deregistration_delay.timeout_seconds
The amount of time for Elastic Load Balancing to wait before deregistering a target. The range is 0–3600 seconds. The default value is 300 seconds.
https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-target-groups.html

Nice 👍

run_test.go Outdated Show resolved Hide resolved
@keroxp
Copy link
Member Author

keroxp commented Jun 27, 2024

@takkyuuplayer @alfnets A comment out was omitted. Could we merge?

Copy link
Contributor

@takkyuuplayer takkyuuplayer left a comment

Choose a reason for hiding this comment

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

LGTM!

@takkyuuplayer takkyuuplayer merged commit ace2c1f into main Jun 27, 2024
3 checks passed
@takkyuuplayer takkyuuplayer deleted the dev-062501 branch June 27, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants