Skip to content

Rework shim logger shutdown process#4162

Merged
estesp merged 1 commit intocontainerd:masterfrom
mxpv:log-fix
Apr 8, 2020
Merged

Rework shim logger shutdown process#4162
estesp merged 1 commit intocontainerd:masterfrom
mxpv:log-fix

Conversation

@mxpv
Copy link
Copy Markdown
Member

@mxpv mxpv commented Apr 7, 2020

A few changes in shim logger process shutdown flow:

  • Close pipes first, so shim logger's s.Scan() exit loops.
  • Rework shutdown flow, so the process receives SIGTERM first (so logger implementations can process cancelled context) with configured timeout, and then kills the process. SIGTERM timeout can be configured with io.containerd.timeout.shim.logger.shutdown timeout key.
  • Reworks error handling a bit.

Fixes: #4159

Signed-off-by: Maksym Pavlenko [email protected]

Comment thread pkg/process/io.go Outdated
Comment thread pkg/process/io.go Outdated
@mxpv mxpv force-pushed the log-fix branch 3 times, most recently from 7c476d1 to 4cbb6b2 Compare April 7, 2020 19:17
@containerd containerd deleted a comment from theopenlab-ci Bot Apr 7, 2020
@containerd containerd deleted a comment from theopenlab-ci Bot Apr 7, 2020
@containerd containerd deleted a comment from theopenlab-ci Bot Apr 7, 2020
@containerd containerd deleted a comment from theopenlab-ci Bot Apr 7, 2020
Copy link
Copy Markdown

@aaithal aaithal left a comment

Choose a reason for hiding this comment

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

thanks for the quick fix!

Copy link
Copy Markdown
Contributor

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

LGTM

@containerd containerd deleted a comment from theopenlab-ci Bot Apr 7, 2020
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #4162 into master will decrease coverage by 3.80%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4162      +/-   ##
==========================================
- Coverage   42.14%   38.33%   -3.81%     
==========================================
  Files         133       90      -43     
  Lines       15209    12705    -2504     
==========================================
- Hits         6410     4871    -1539     
+ Misses       7870     7174     -696     
+ Partials      929      660     -269     
Flag Coverage Δ
#linux ?
#windows 38.33% <ø> (+0.35%) ⬆️
Impacted Files Coverage Δ
archive/tar_opts.go 11.76% <0.00%> (-47.06%) ⬇️
cio/io.go 1.40% <0.00%> (-45.08%) ⬇️
snapshots/native/native.go 1.73% <0.00%> (-40.70%) ⬇️
archive/tar.go 18.95% <0.00%> (-29.64%) ⬇️
metadata/snapshot.go 33.18% <0.00%> (-14.40%) ⬇️
metadata/adaptors.go 39.04% <0.00%> (-9.53%) ⬇️
oci/spec_opts.go 32.50% <0.00%> (-2.25%) ⬇️
content/local/writer.go 57.69% <0.00%> (-0.97%) ⬇️
gc/scheduler/scheduler.go 66.34% <0.00%> (-0.97%) ⬇️
remotes/docker/config/config_unix.go
... and 43 more

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 23fc859...ed63f48. Read the comment docs.

Comment thread pkg/process/io.go
return nil, err
}

serr, err := newPipe()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we should close pipe if meet error. since it is not related to this issue, it can be follow-up pr.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Will follow up.

Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

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.

shim logger processes never gets a SIGTERM

6 participants