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.

Sign up for GitHub

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

Jump to bottom

fix(executor): More logs for PNS sidecar termination. #5627 #5683

Merged
merged 9 commits into from
Apr 16, 2021
Merged

fix(executor): More logs for PNS sidecar termination. #5627 #5683

merged 9 commits into from
Apr 16, 2021

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Apr 15, 2021

No description provided.

@alexec
fix(executor): More logs for PNS sidecar termination. argoproj#5627
df312cd 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
alexec
alexec commented Apr 15, 2021
View reviewed changes
Makefile
@@ -129,12 +129,12 @@ define protoc
perl -i -pe 's|argoproj/argo-workflows/|argoproj/argo-workflows/v3/|g' `echo "$(1)" | sed 's/proto/pb.go/g'`

endef
# docker_build,image_name
# docker_build,target,image_name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes a bug in build where locally built argoexec did not get imported into k3s

@alexec alexec linked an issue Apr 15, 2021 that may be closed by this pull request
TestSignalsSuite/TestInjectedSidecar fails too often to be flakey IMHO #5627
Closed
@codecov
Copy link

codecov bot commented Apr 15, 2021 •
edited
Loading

Codecov Report

Merging #5683 (82c551d) into master (f6be569) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 82c551d differs from pull request most recent head 7fc195f. Consider uploading reports for the commit 7fc195f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5683      +/-   ##
==========================================
- Coverage   46.91%   46.90%   -0.02%     
==========================================
  Files         244      244              
  Lines       15209    15209              
==========================================
- Hits         7136     7134       -2     
+ Misses       7168     7167       -1     
- Partials      905      908       +3
Impacted Files Coverage Δ
workflow/executor/executor.go 16.23% <0.00%> (ø)
cmd/argoexec/commands/emissary.go 48.43% <0.00%> (-1.57%) ⬇️
cmd/argo/commands/get.go 56.31% <0.00%> (ø)

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 f6be569...7fc195f. Read the comment docs.

@alexec
Copy link
Contributor Author

alexec commented Apr 15, 2021

Failed PNS build:

https://github.com/argoproj/argo-workflows/pull/5683/checks?check_run_id=2355710905

34m          logs | �[msidecar-injected-8l77l wait time="2021-04-15T18:38:18.225Z" level=info msg="guessing container \"main\" PID is 28"
�[34m          logs | �[msidecar-injected-8l77l wait time="2021-04-15T18:38:18.225Z" level=info msg="Waiting for \"main\" pid 28 to complete"
�[34m          logs | �[msidecar-injected-8l77l wait time="2021-04-15T18:38:18.232Z" level=info msg="secured root for pid 28 root: sh"

alexec added 6 commits April 15, 2021 12:03
@alexec
added ALEX logging
368e496 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
remove IfNotPresent
eb85e4b 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
print logs
f9abe08 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
trigger ci
373b6b2 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
switch to using k8sapi
d06ca3f 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
trigger ci
eb36e89 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec marked this pull request as ready for review April 15, 2021 21:52
@alexec alexec added this to the v3.0 milestone Apr 15, 2021
@alexec
Copy link
Contributor Author

alexec commented Apr 15, 2021

@sarabala1979 please review.

alexec
alexec commented Apr 16, 2021
View reviewed changes
workflow/executor/pns/pns.go
@@ -157,32 +157,6 @@ func (p *PNSExecutor) Wait(ctx context.Context, containerNames []string) error {
time.Sleep(1 * time.Second)
}

OUTER:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is worthwhile getting this to work as the performance benefit is much smaller than the bugginess.

@alexec alexec requested a review from whynowy April 16, 2021 18:30
dinever
dinever reviewed Apr 16, 2021
View reviewed changes
workflow/executor/pns/pns.go Outdated
_ = prevInfo.Close()
}
p.pidFileHandles[pid] = fs
log.Infof("ALEX secured root for pid %d root: %s (%q)", pid, proc.Executable(), fs.Name())
Copy link
Member

@dinever dinever Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Just curious. Is this part ALEX .. for debugging purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops...

alexec added 2 commits April 16, 2021 13:25
@alexec
Merge branch 'master' into pns
514c7a9 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
ok
7fc195f 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec merged commit 91c08cd into argoproj:master Apr 16, 2021
@alexec alexec deleted the pns branch April 16, 2021 23:05
@simster7 simster7 mentioned this pull request Apr 19, 2021
Closed
50 tasks
alexec added a commit that referenced this pull request Apr 19, 2021
@alexec
fix(executor): More logs for PNS sidecar termination. #5627 (#5683)
9492e12 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dinever dinever dinever left review comments

@sarabala1979 sarabala1979 sarabala1979 approved these changes

@whynowy whynowy Awaiting requested review from whynowy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0
Development

Successfully merging this pull request may close these issues.

TestSignalsSuite/TestInjectedSidecar fails too often to be flakey IMHO
3 participants
@alexec @dinever @sarabala1979