Skip to content

Update ttrpc to support context timeout.#2930

Merged
estesp merged 1 commit intocontainerd:masterfrom
cpuguy83:update_ttrpc
Jan 15, 2019
Merged

Update ttrpc to support context timeout.#2930
estesp merged 1 commit intocontainerd:masterfrom
cpuguy83:update_ttrpc

Conversation

@cpuguy83
Copy link
Copy Markdown
Member

At a bit of a loss on how to write a test case for this.
Existing ttrpc libs will already honor a canceled context if it's canceled before dispatch in the ttrpc client is called.
Other methods on the containerd client go through grpc which is already going to return early on a cancelled context.
Thought of writing a custom shim implementation but then that's not really testing much, certainly not anything more than what's being tested in the ttrpc repo already.

Thoughts?

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jan 15, 2019

"failed to pull and unpack image \"docker.io/library/nginx:latest\": failed to copy: httpReaderSeeker: failed open: could not fetch content descriptor sha256:ea57c53235dfe1ae1db219ca7cda6210c8f875367bcb892fdc6d86c047174f3d (application/vnd.docker.image.rootfs.diff.tar.gzip) from remote: not found"

travis-ci testing case fails because the docker registry is unstable I guess :P
we run into the issue several times yesterday.

@codecov-io
Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2930   +/-   ##
=======================================
  Coverage   44.02%   44.02%           
=======================================
  Files         102      102           
  Lines       10871    10871           
=======================================
  Hits         4786     4786           
  Misses       5351     5351           
  Partials      734      734
Flag Coverage Δ
#linux 47.65% <ø> (ø) ⬆️
#windows 41.23% <ø> (ø) ⬆️

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 e30bba5...0befa45. Read the comment docs.

1 similar comment
@codecov-io
Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2930   +/-   ##
=======================================
  Coverage   44.02%   44.02%           
=======================================
  Files         102      102           
  Lines       10871    10871           
=======================================
  Hits         4786     4786           
  Misses       5351     5351           
  Partials      734      734
Flag Coverage Δ
#linux 47.65% <ø> (ø) ⬆️
#windows 41.23% <ø> (ø) ⬆️

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 e30bba5...0befa45. Read the comment docs.

@crosbymichael
Copy link
Copy Markdown
Member

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

@estesp estesp merged commit 804faea into containerd:master Jan 15, 2019
@cpuguy83 cpuguy83 deleted the update_ttrpc branch January 15, 2019 16:39
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.

5 participants