Skip to content

v1:Respect the shim_debug flag when load tasks#3276

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
darfux:v1_respect_shim_debug
May 13, 2019
Merged

v1:Respect the shim_debug flag when load tasks#3276
crosbymichael merged 1 commit intocontainerd:masterfrom
darfux:v1_respect_shim_debug

Conversation

@darfux
Copy link
Copy Markdown
Contributor

@darfux darfux commented May 13, 2019

Currently when we restart containerd it will load all tasks with shim
logs whether the shim_debug is set or not.

Signed-off-by: Li Yuxuan [email protected]

@darfux darfux force-pushed the v1_respect_shim_debug branch 3 times, most recently from a3910bb to a30a190 Compare May 13, 2019 04:31
@codecov-io
Copy link
Copy Markdown

codecov-io commented May 13, 2019 •
edited
Loading

Codecov Report

Merging #3276 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3276   +/-   ##
======================================
  Coverage    44.4%   44.4%           
======================================
  Files         113     113           
  Lines       12231   12231           
======================================
  Hits         5431    5431           
  Misses       5966    5966           
  Partials      834     834
Flag Coverage Δ
#linux 48.33% <ø> (ø) ⬆️
#windows 39.66% <ø> (ø) ⬆️

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 c0d0fc3...66036d9. Read the comment docs.

fuweid
fuweid approved these changes May 13, 2019
View reviewed changes
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

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented May 13, 2019

I think we may want to forward to /dev/null if debug is not set so that we make sure the pipes are flushed and don't backup and block the shim process

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented May 13, 2019

I think we may want to forward to /dev/null if debug is not set so that we make sure the pipes are flushed and don't backup and block the shim process

@crosbymichael Great idea! So should we also forward to /dev/null when the shim process starting ?

containerd/runtime/v1/shim/client/client.go

Lines 70 to 84 in 0485499

if debug {
stdoutLog, err = v1.OpenShimStdoutLog(ctx, config.WorkDir)
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to create stdout log")
}
stderrLog, err = v1.OpenShimStderrLog(ctx, config.WorkDir)
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to create stderr log")
}
go io.Copy(os.Stdout, stdoutLog)
go io.Copy(os.Stderr, stderrLog)
}

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented May 13, 2019

@darfux ya, lets go ahead and make that change as well

@darfux
v1: Respect the shim_debug flag when load tasks
66036d9 
Currently when we restart containerd it will load all tasks with shim
logs whether the `shim_debug` is set or not.

Signed-off-by: Li Yuxuan <[email protected]>
@darfux darfux force-pushed the v1_respect_shim_debug branch from a30a190 to 66036d9 Compare May 13, 2019 15:51
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented May 13, 2019

Build succeeded.

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented May 13, 2019

@crosbymichael I found that the stdout&stderr of shim will be set to nil when starting if debug is not set. So I just forward to /dev/null when load tasks. :D

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented May 13, 2019

LGTM

@crosbymichael crosbymichael merged commit 7acdb16 into containerd:master May 13, 2019
@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented May 13, 2019

thanks @darfux

@darfux darfux deleted the v1_respect_shim_debug branch May 14, 2019 01:56
@darfux darfux restored the v1_respect_shim_debug branch August 8, 2019 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fuweid fuweid fuweid approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@darfux @codecov-io @crosbymichael @fuweid