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

fix(executor): Delegate PNS wait to K8SAPI executor. #5307

Merged
merged 3 commits into from
Mar 6, 2021

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Mar 5, 2021

No description provided.

Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
}

OUTER:
for _, containerName := range containerNames {
Copy link
Contributor Author

@alexec alexec Mar 5, 2021

Choose a reason for hiding this comment

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

Very rarely, containers can successfully complete, but the Kubernetes API does not yet know about it, so kubectl logs will then fail, resulting in the wait container failing.

I don't see any easy way around this, so this reverts us to how v2.12.

It's a shame to lose this performance boost, but a much worse shame to have unpredictable workflow failures.

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec marked this pull request as ready for review March 5, 2021 17:26
@alexec alexec requested a review from sarabala1979 March 5, 2021 17:26
@alexec alexec added this to the v3.0 milestone Mar 5, 2021
@sarabala1979 sarabala1979 self-assigned this Mar 5, 2021
@alexec
Copy link
Contributor Author

alexec commented Mar 5, 2021

@sarabala1979 bump!

@@ -58,12 +58,7 @@ func (k *K8sAPIExecutor) GetExitCode(ctx context.Context, containerName string)
// Wait for the container to complete
func (k *K8sAPIExecutor) Wait(ctx context.Context, containerNames, sidecarNames []string) error {
Copy link
Member

@sarabala1979 sarabala1979 Mar 6, 2021

Choose a reason for hiding this comment

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

Don't we terminate Sidecar containers also?

@alexec alexec merged commit b048875 into argoproj:master Mar 6, 2021
@alexec alexec deleted the dev-pns branch March 6, 2021 00:40
This was referenced Mar 8, 2021
@simster7 simster7 mentioned this pull request Apr 19, 2021
50 tasks
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.

None yet

2 participants