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): Always poll for Docker injected sidecars. Resolves #5448 #5479

Merged
merged 9 commits into from
Mar 24, 2021

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Mar 22, 2021

No description provided.

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #5479 (74aa444) into master (95bd1cc) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 74aa444 differs from pull request most recent head 4fcffc7. Consider uploading reports for the commit 4fcffc7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5479      +/-   ##
==========================================
- Coverage   46.69%   46.67%   -0.03%     
==========================================
  Files         240      240              
  Lines       14994    14994              
==========================================
- Hits         7002     6998       -4     
- Misses       7093     7100       +7     
+ Partials      899      896       -3     
Impacted Files Coverage Δ
server/workflow/workflow_server.go 40.45% <0.00%> (-2.28%) ⬇️
cmd/argo/commands/get.go 57.33% <0.00%> (+0.66%) ⬆️
cmd/argoexec/commands/emissary.go 50.00% <0.00%> (+1.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95bd1cc...4fcffc7. Read the comment docs.

@alexec alexec linked an issue Mar 22, 2021 that may be closed by this pull request
@alexec
Copy link
Contributor Author

alexec commented Mar 22, 2021

functional_test.go:310: timeout after 30s waiting for condition

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec requested a review from whynowy March 22, 2021 18:39
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
Copy link
Contributor Author

alexec commented Mar 22, 2021

artifacts_test.go:52: timeout after 30s waiting for condition

pns

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@@ -185,6 +185,8 @@ func (d *DockerExecutor) GetExitCode(ctx context.Context, containerName string)
}

func (d *DockerExecutor) Wait(ctx context.Context, containerNames []string) error {
ctx, cancel := context.WithCancel(ctx) // stop the polling when we are no longer waiting
defer cancel()
go func() {
err := d.pollContainerIDs(ctx, containerNames)
Copy link
Member

@whynowy whynowy Mar 22, 2021

Choose a reason for hiding this comment

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

If pollContainerIDs returns with a timeout error, it could stay at for !d.haveContainers(containerNames) forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

poll now runs for ever

@@ -185,6 +185,8 @@ func (d *DockerExecutor) GetExitCode(ctx context.Context, containerName string)
}

func (d *DockerExecutor) Wait(ctx context.Context, containerNames []string) error {
ctx, cancel := context.WithCancel(ctx) // stop the polling when we are no longer waiting
Copy link
Member

Choose a reason for hiding this comment

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

It seems to be useless here to do WithCancel(ctx). If the passed in ctx is cancelable, then there's no need to do it; If the passed in ctx is not cancelable, then I don't see a way could hit ctx.Done().

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec changed the title fix(executor): Allow 30s for Docker/PNS polling. Resolves #5448 fix(executor): Always poll for Docker/PNS injected sidecars. Resolves #5448 Mar 23, 2021
@alexec
Copy link
Contributor Author

alexec commented Mar 23, 2021

pns: artifacts_test.go:52: timeout after 30s waiting for condition
emissary: cluster_workflow_template_test.go:55: timeout after 30s waiting for condition

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec changed the title fix(executor): Always poll for Docker/PNS injected sidecars. Resolves #5448 fix(executor): Always poll for Docker injected sidecars. Resolves #5448 Mar 23, 2021
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec marked this pull request as ready for review March 24, 2021 18:54
@alexec
Copy link
Contributor Author

alexec commented Mar 24, 2021

@whynowy ready for review again.

I did not make your change - I'd prefer to wait forever, I don't think we ever can have a case now that we don't get all container names.

I've removed the PNS changes.

Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

LGTM

@alexec alexec added this to the v3.0 milestone Mar 24, 2021
@alexec
Copy link
Contributor Author

alexec commented Mar 24, 2021

Set milestone as v3.0 for inclusing in v3.0.0-rc10.

@alexec alexec merged commit 865b1fe into master Mar 24, 2021
@alexec alexec deleted the dev-docker branch March 24, 2021 19:10
This was referenced Mar 29, 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.

v3.0: wait containers do not exit
2 participants