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 retry when kill container #7404

Closed
wants to merge 1 commit into from

Conversation

henrywangx
Copy link
Contributor

Since sometimes the connection between the APIServer is not stable. We don't want to rerun for this case.
So add retry to APIServer.

  • Run make pre-commit -B to fix codegen, lint, and commit message problems.

@henrywangx henrywangx changed the title add retry when kill container and ignore error when kill sidecar failed add retry when kill container Dec 15, 2021
cmd/argoexec/commands/wait.go Show resolved Hide resolved
cmd/argoexec/commands/wait.go Outdated Show resolved Hide resolved
@henrywangx henrywangx force-pushed the xiowang branch 2 times, most recently from 26663e0 to f0db246 Compare December 17, 2021 13:14
@terrytangyuan
Copy link
Member

Please fix the failing builds.

@henrywangx
Copy link
Contributor Author

henrywangx commented Dec 20, 2021

Please fix the failing builds.

@terrytangyuan Done

@henrywangx henrywangx force-pushed the xiowang branch 3 times, most recently from 3aa6e8a to 8de0afa Compare December 22, 2021 02:28
@alexec alexec changed the title add retry when kill container feat: add retry when kill container Jan 19, 2022
@alexec
Copy link
Contributor

alexec commented Jan 19, 2022

@terrytangyuan ready to re-review?

Comment on lines +120 to +122
if err != nil {
return false, err
}
Copy link
Member

Choose a reason for hiding this comment

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

We probably only want to retry for transient errors?

@@ -31,7 +35,21 @@ func waitContainer(ctx context.Context) error {
stats.StartStatsTicker(5 * time.Minute)

defer func() {
if err := wfExecutor.KillSidecars(ctx); err != nil {
// Killing sidecar containers
retryCnt := 0
Copy link
Member

Choose a reason for hiding this comment

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

We currently don't have retry counter in other places. It might be better to add later in all places where we are retrying.

@stale
Copy link

stale bot commented Feb 7, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the problem/stale This has not had a response in some time label Feb 7, 2022
@terrytangyuan
Copy link
Member

Any update on this?

@stale stale bot removed the problem/stale This has not had a response in some time label Feb 7, 2022
@henrywangx
Copy link
Contributor Author

henrywangx commented Feb 10, 2022

@terrytangyuan I have removed retryCnt. But I found the master branch has changed a lot, I can't fix the CI. Just close this PR.

@henrywangx henrywangx closed this Feb 10, 2022
@tooptoop4

This comment was marked as spam.

@agilgur5 agilgur5 added area/controller Controller issues, panics problem/stale This has not had a response in some time labels Oct 22, 2024
@agilgur5
Copy link

where @henrywangx

The kill command will get re-queued these days if it fails, so this is no longer necessary

@agilgur5 agilgur5 removed the problem/stale This has not had a response in some time label Oct 22, 2024
@argoproj argoproj locked as resolved and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/controller Controller issues, panics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants