Don't cancel context passed to `OpenFifo`. by Random-Liu · Pull Request #2944 · containerd/containerd

Random-Liu · 2019-01-23T00:20:27Z

We can have a better fix after containerd/fifo#18 is addressed.

More details at #2944 (comment).
Signed-off-by: Lantao Liu [email protected]

Random-Liu · 2019-01-23T00:34:32Z

This PR also removed the unused linter, because it has been removed from the upstream. alecthomas/gometalinter@96c7e2a

I keep getting this error without doing that:

$ make check
+ proto-fmt
+ check
gometalinter --config .gometalinter.json ./...
gometalinter: error: unknown linters: unused
Makefile:109: recipe for target 'check' failed
make: *** [check] Error 1

Random-Liu · 2019-01-23T00:46:46Z

Hm, actually the change alecthomas/gometalinter@96c7e2a broke a ton of things:

gometalinter --config .gometalinter.json ./...
archive\tar_windows.go:77:24:warning: error strings should not be capitalized (ST1005) (staticcheck)
archive\tar_windows.go:133:2:warning: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008) (staticcheck)
cmd\containerd\command\service_windows.go:391:2:warning: should use for range instead of for { select {} } (S1000) (staticcheck)
container_test.go:139:24:warning: no value of type uint32 is less than 0 (SA4003) (staticcheck)
container_test.go:438:5:warning: no value of type uint32 is less than 0 (SA4003) (staticcheck)
container_test.go:788:24:warning: no value of type uint32 is less than 0 (SA4003) (staticcheck)
content\local\locks.go:50:2:warning: unnecessary guard around call to delete (S1033) (staticcheck)
diff\apply\apply.go:61:14:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
diff\lcow\lcow.go:102:14:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
diff\windows\windows.go:95:14:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
filters\scanner.go:188:2:warning: redundant return statement (S1023) (staticcheck)
import.go:102:2:warning: should merge variable declaration with assignment on next line (S1021) (staticcheck)
metadata\content.go:782:8:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
metadata\db.go:179:31:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
metadata\db.go:331:40:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
metadata\db.go:346:21:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
metadata\db.go:354:16:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
metadata\snapshot.go:631:8:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
pkg\progress\escape.go:22:2:warning: const red is unused (U1000) (staticcheck)
plugin\plugin.go:45:2:warning: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008) (staticcheck)
remotes\docker\auth.go:82:18:warning: should use strings.ContainsRune(" \t\"(),/:;<=>?@[]\\{}", rune(c)) instead (S1003) (staticcheck)
remotes\docker\auth.go:83:6:warning: should use strings.ContainsRune(" \t\r\n", rune(c)) instead (S1003) (staticcheck)
runtime\v2\shim\shim_windows.go:111:2:warning: should use for range instead of for { select {} } (S1000) (staticcheck)
runtime\v2\shim\util_windows.go:57:26:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
runtime\v2\shim\util_windows.go:74:6:warning: should use time.Since instead of time.Now().Sub (S1012) (staticcheck)
services\containers\local.go:150:4:warning: should replace loop with fieldpaths = append(fieldpaths, req.UpdateMask.Paths...) (S1011) (staticcheck)
services\images\local.go:140:3:warning: should replace loop with fieldpaths = append(fieldpaths, req.UpdateMask.Paths...) (S1011) (staticcheck)
Makefile:109: recipe for target 'check' failed
mingw32-make: *** [check] Error 1

I'll make a quick change to stick to the commit before alecthomas/gometalinter@96c7e2a for this PR...

codecov-io · 2019-01-23T01:10:41Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #2944   +/-   ##
=======================================
  Coverage   44.02%   44.02%           
=======================================
  Files         102      102           
  Lines       10870    10870           
=======================================
  Hits         4785     4785           
  Misses       5351     5351           
  Partials      734      734

Flag	Coverage Δ
#linux	`47.65% <ø> (ø)`	⬆️
#windows	`41.24% <ø> (ø)`	⬆️

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 35582cb...92b89de. Read the comment docs.

codecov-io · 2019-01-23T01:10:46Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2944      +/-   ##
==========================================
- Coverage   47.64%   44.02%   -3.63%     
==========================================
  Files          92      102      +10     
  Lines        8513    10870    +2357     
==========================================
+ Hits         4056     4785     +729     
- Misses       3730     5351    +1621     
- Partials      727      734       +7

Flag	Coverage Δ
#linux	`47.65% <ø> (ø)`	⬆️
#windows	`41.24% <ø> (?)`

Impacted Files	Coverage Δ
snapshots/native/native.go	`43.04% <0%> (-9.99%)`	⬇️
metadata/snapshot.go	`45.8% <0%> (-8.96%)`	⬇️
archive/tar.go	`43.79% <0%> (-7.07%)`	⬇️
metadata/containers.go	`47.97% <0%> (-6.62%)`	⬇️
content/local/writer.go	`57.84% <0%> (-6.36%)`	⬇️
content/local/store.go	`48.51% <0%> (-5.03%)`	⬇️
archive/tar_opts.go	`28.57% <0%> (-4.77%)`	⬇️
archive/compression/compression.go	`58.69% <0%> (-4.7%)`	⬇️
metadata/images.go	`58.46% <0%> (-4.7%)`	⬇️
metadata/buckets.go	`56.33% <0%> (-4.6%)`	⬇️
... and 59 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 aee74ad...26ab393. Read the comment docs.

Random-Liu · 2019-01-23T01:22:45Z

IIUC, the timeout context added in #2877 is mainly for the blocking OpenFifo calls. However, it is fine to add it for non-blocking calls as well, because:

We hold both writer and reader side in containerd, both goroutines in non-blocking OpenFifo should finish fast (<30s)
If anything really goes wrong, a 30s timeout helps us unblock the stall non-blocking OpenFifo goroutines and fail loudly.

fuweid · 2019-01-23T01:38:36Z

I think the change is reasonable because the fifo non-blocking goroutine is not in the request scope. SGTM.

@Random-Liu I see that exec process still uses the request context. do we need to update it too?

https://github.com/containerd/containerd/blob/365b163faf9be9da99f22c10a8295286c5229ae1/runtime/v1/linux/proc/exec.go#L212-L220

Random-Liu · 2019-01-23T02:27:11Z

@fuweid Hm, actually it seems like a bug to me. We don't cancel the context in the regular code path. And although there can only be one Create, there can be many execs at the same time.

Say we run 1000 short-live execs in a container one by one in 30s, there will be 1000 goroutines running at the same time spawned by the timeout context.

I really think we should address containerd/fifo#18 then. @crosbymichael

Random-Liu · 2019-01-23T03:14:57Z

I updated the PR for the issue mentioned in #2944 (comment).

The new change:

use context.Background() for O_NONBLOCK calls to avoid the uncancelled context. We can do this because:
- The initial timeout Add timeout and cancel to shim fifo open #2877 was mainly added for blocking calls, not O_NONBLOCK calls.
- We only use O_NONBLOCK for stdin, and we always open both side of the stdin pipe in the containerd-shim, so OpenFifo shouldn't block forever.
- Even if we only hold one side, we do need to wait forever in the background until the client opens its side.
Remove unnecessary O_NONBLOCK, e.g. O_RDWR has to be blocked (see https://github.com/containerd/fifo/blob/master/fifo.go#L65)

If we are ok with this approach, we should also fix all the cherry-picks of #2877 because of #2944 (comment)

estesp · 2019-01-23T16:11:01Z

Just FYI; now that #2948 is merged, you can drop commit 9f669c91bd3740d3bb81f7df115ecbb89e3ac355 from this PR (well, assuming you can easily rebase on master)

crosbymichael · 2019-01-23T16:12:44Z

You can remove this now and rebase

cpuguy83 · 2019-01-23T18:16:47Z

Can you update the commit message to include why this is being done?
Otherwise changes seem correct, we don't want to bind the non-blocking opens to the request lifecycle.

crosbymichael · 2019-01-23T18:18:45Z

LGTM

Signed-off-by: Lantao Liu <[email protected]>

Random-Liu · 2019-01-23T18:20:08Z

Can you update the commit message to include why this is being done?

Done.

dmcgowan · 2019-01-23T18:51:04Z

LGTM

Random-Liu mentioned this pull request Jan 23, 2019

Lint is broken #2945

Closed

Random-Liu force-pushed the fix-stdin-close branch from b0cfcbc to 92b89de Compare January 23, 2019 00:54

Random-Liu force-pushed the fix-stdin-close branch from 92b89de to 365b163 Compare January 23, 2019 01:18

Random-Liu force-pushed the fix-stdin-close branch 2 times, most recently from 59f1368 to cadb6a4 Compare January 23, 2019 03:11

Random-Liu force-pushed the fix-stdin-close branch from cadb6a4 to 9f669c9 Compare January 23, 2019 03:20

crosbymichael reviewed Jan 23, 2019

View reviewed changes

Random-Liu force-pushed the fix-stdin-close branch from 9f669c9 to d98944f Compare January 23, 2019 17:49

Use context.Background for O_NONBLOCK OpenFifo.

26ab393

Signed-off-by: Lantao Liu <[email protected]>

Random-Liu force-pushed the fix-stdin-close branch from d98944f to 26ab393 Compare January 23, 2019 18:19

dmcgowan merged commit 3762378 into containerd:master Jan 23, 2019

Random-Liu deleted the fix-stdin-close branch January 23, 2019 22:13

Random-Liu mentioned this pull request Jan 23, 2019

[release/1.2] cherrypick: Use context.Background for O_NONBLOCK OpenFifo. #2955

Merged

cpuguy83 mentioned this pull request May 6, 2019

[release/1.2 backport] Update ttrpc #3250

Merged

Conversation

Random-Liu commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Random-Liu commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Random-Liu commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 23, 2019

Codecov Report

Uh oh!

codecov-io commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Random-Liu commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fuweid commented Jan 23, 2019

Uh oh!

Random-Liu commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Random-Liu commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estesp commented Jan 23, 2019

Uh oh!

crosbymichael Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

Random-Liu Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jan 23, 2019

Uh oh!

crosbymichael commented Jan 23, 2019

Uh oh!

Random-Liu commented Jan 23, 2019

Uh oh!

dmcgowan commented Jan 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Random-Liu commented Jan 23, 2019 •

edited

Loading

Random-Liu commented Jan 23, 2019 •

edited

Loading

Random-Liu commented Jan 23, 2019 •

edited

Loading

codecov-io commented Jan 23, 2019 •

edited

Loading

Random-Liu commented Jan 23, 2019 •

edited

Loading

Random-Liu commented Jan 23, 2019 •

edited

Loading

Random-Liu commented Jan 23, 2019 •

edited

Loading