Improve host fallback behaviour in docker remote by awprice · Pull Request #3868 · containerd/containerd

awprice · 2019-12-03T00:44:30Z

This commit improves the fallback behaviour when resolving and
fetching images with multiple hosts. If an error is encountered
when resolving and fetching images, and more than one host is being
used, we will try the same operation on the next host. The error
from the first host is preserved so that if all hosts fail, we can
display the error from the first host.

fixes #3850

Signed-off-by: Alex Price [email protected]

awprice · 2019-12-03T00:45:23Z

Had a little bit of difficulty coming up with good tests for this, any pointers would be helpful. I definitely think tests are needed to ensure this behaviour isn't lost.

theopenlab-ci · 2019-12-03T00:51:01Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 40s (non-voting)

remotes/docker/fetcher.go

dmcgowan · 2019-12-03T00:59:34Z

Had a little bit of difficulty coming up with good tests for this, any pointers would be helpful. I definitely think tests are needed to ensure this behaviour isn't lost.

The tests have a common pattern, but most rely on returning an HTTP code. You can try either returning a panic in the handler or wrapping the listener to return a connection which closes. TLS to non-TLS endpoint could simulate the error too.

remotes/docker/fetcher.go

codecov-io · 2019-12-03T01:43:52Z

Codecov Report

Merging #3868 into master will decrease coverage by 3.25%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3868      +/-   ##
==========================================
- Coverage   45.72%   42.46%   -3.26%     
==========================================
  Files         117      130      +13     
  Lines       11765    14702    +2937     
==========================================
+ Hits         5379     6243     +864     
- Misses       5462     7538    +2076     
+ Partials      924      921       -3

Flag	Coverage Δ
#linux	`45.85% <64.7%> (+0.13%)`	⬆️
#windows	`38% <66.66%> (?)`

Impacted Files	Coverage Δ
remotes/docker/resolver.go	`56.25% <100%> (-0.29%)`	⬇️
remotes/docker/fetcher.go	`59.52% <58.82%> (+17.02%)`	⬆️
archive/tar_opts.go	`58.82% <0%> (-12.61%)`	⬇️
snapshots/native/native.go	`42.42% <0%> (-10.27%)`	⬇️
cio/io.go	`46.47% <0%> (-8.53%)`	⬇️
archive/tar.go	`48.18% <0%> (-8.51%)`	⬇️
metadata/snapshot.go	`48.47% <0%> (-7.19%)`	⬇️
metadata/containers.go	`49.84% <0%> (-6.26%)`	⬇️
content/local/writer.go	`58.65% <0%> (-5.55%)`	⬇️
content/local/store.go	`49.52% <0%> (-5.29%)`	⬇️
... and 76 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 ff91f22...a022c21. Read the comment docs.

theopenlab-ci · 2019-12-03T01:57:14Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 6m 07s (non-voting)

awprice · 2019-12-03T02:05:31Z

Had a little bit of difficulty coming up with good tests for this, any pointers would be helpful. I definitely think tests are needed to ensure this behaviour isn't lost.

The tests have a common pattern, but most rely on returning an HTTP code. You can try either returning a panic in the handler or wrapping the listener to return a connection which closes. TLS to non-TLS endpoint could simulate the error too.

Thanks for the pointers, working on tests now.

awprice · 2019-12-03T03:37:37Z

Tests added. They aim to test two cases:

Falling back to the second host when the first is unresponsive/not running
Falling back to the second host when communicating over TLS to the first non-TLS host

theopenlab-ci · 2019-12-03T03:46:44Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 33s (non-voting)

remotes/docker/resolver_test.go

theopenlab-ci · 2019-12-12T23:58:58Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 24s (non-voting)

This commit improves the fallback behaviour when resolving and fetching images with multiple hosts. If an error is encountered when resolving and fetching images, and more than one host is being used, we will try the same operation on the next host. The error from the first host is preserved so that if all hosts fail, we can display the error from the first host. fixes containerd#3850 Signed-off-by: Alex Price <[email protected]>

theopenlab-ci · 2019-12-13T00:47:59Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 20s (non-voting)

fuweid

LGTM

dmcgowan

LGTM

coreyjohnston · 2020-01-20T05:01:50Z

Its been over a month since 1.3.2 was released. Do we know when we're expecting the next release which will include this?

dmcgowan reviewed Dec 3, 2019

View reviewed changes

remotes/docker/fetcher.go Outdated Show resolved Hide resolved

ktock reviewed Dec 3, 2019

View reviewed changes

remotes/docker/fetcher.go Outdated Show resolved Hide resolved

awprice force-pushed the issue-3850 branch from 74a93f1 to a2ae0f8 Compare December 3, 2019 01:50

awprice force-pushed the issue-3850 branch from a2ae0f8 to 0979653 Compare December 3, 2019 03:35

awprice force-pushed the issue-3850 branch from 0979653 to bfcd432 Compare December 3, 2019 03:39

fuweid reviewed Dec 11, 2019

View reviewed changes

remotes/docker/resolver_test.go Outdated Show resolved Hide resolved

awprice force-pushed the issue-3850 branch from bfcd432 to 324d0b2 Compare December 12, 2019 23:53

awprice force-pushed the issue-3850 branch from 324d0b2 to a022c21 Compare December 13, 2019 00:43

fuweid approved these changes Dec 13, 2019

View reviewed changes

crosbymichael requested a review from dmcgowan December 17, 2019 01:38

dmcgowan approved these changes Dec 17, 2019

View reviewed changes

dmcgowan merged commit 5661214 into containerd:master Dec 17, 2019

awprice deleted the issue-3850 branch February 7, 2020 02:39

awprice mentioned this pull request Feb 10, 2020

[release/1.3 backport] Improve host fallback behaviour in docker remote #4007

Merged

weikinhuang mentioned this pull request Sep 4, 2020

Registry mirror fallbacks immediately errors if response code is 5xx instead of connection timeout/fail #4531

Closed

teemuteemu mentioned this pull request Sep 18, 2020

Set resolver to pull the image from next registry #4575

Closed

jedevc mentioned this pull request Jul 24, 2023

docker: return most relevant error from docker resolution #8861

Merged

Conversation

awprice commented Dec 3, 2019

Uh oh!

awprice commented Dec 3, 2019

Uh oh!

theopenlab-ci bot commented Dec 3, 2019

Uh oh!

Uh oh!

dmcgowan commented Dec 3, 2019

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

theopenlab-ci bot commented Dec 3, 2019

Uh oh!

awprice commented Dec 3, 2019

Uh oh!

awprice commented Dec 3, 2019

Uh oh!

theopenlab-ci bot commented Dec 3, 2019

Uh oh!

Uh oh!

theopenlab-ci bot commented Dec 12, 2019

Uh oh!

theopenlab-ci bot commented Dec 13, 2019

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

coreyjohnston commented Jan 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Dec 3, 2019 •

edited

Loading