Skip to content

Try next mirror in case of non-404 errors, too #5275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dmcgowan merged 1 commit into from
May 19, 2021
Merged

Try next mirror in case of non-404 errors, too #5275

dmcgowan merged 1 commit into from
May 19, 2021

Conversation

@haslersn
Copy link
Contributor

@haslersn haslersn commented Mar 26, 2021

This PR is based on #4575 (comment).

Fixes #4531
Closes #4575

@k8s-ci-robot
Copy link

k8s-ci-robot commented Mar 26, 2021

Hi @haslersn. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@haslersn haslersn force-pushed the fix-4531 branch 2 times, most recently from 63be998 to e3d42f5 Compare March 26, 2021 20:32
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 26, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 27, 2021

Build succeeded.

ktock
ktock reviewed Mar 30, 2021
View reviewed changes
Copy link
Member

@ktock ktock left a comment •
edited
Loading

Choose a reason for hiding this comment

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

overall LGTM but some minor nits.

remotes/docker/resolver.go Outdated
Copy link
Member

@ktock ktock Mar 30, 2021

Choose a reason for hiding this comment

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

nit: lastErr doesn's seems to be the correct name as this actually indicates the first error 🤔

Copy link
Contributor Author

@haslersn haslersn Mar 30, 2021

Choose a reason for hiding this comment

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

Right, but that seems to already be the case at:

Should I rename it?

Copy link
Member

@ktock ktock Mar 30, 2021

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

@haslersn haslersn Apr 4, 2021

Choose a reason for hiding this comment

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

Done

remotes/docker/resolver.go Outdated
Copy link
Member

@ktock ktock Mar 30, 2021

Choose a reason for hiding this comment

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

nit: #4575 (comment) should be commented somewhere.

Copy link
Contributor Author

@haslersn haslersn Apr 4, 2021

Choose a reason for hiding this comment

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

Done

@mariuskiessling mariuskiessling mentioned this pull request Apr 4, 2021
Closed
@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 4, 2021

Build succeeded.

@ktock
Copy link
Member

ktock commented Apr 5, 2021

linter is not happy.

@ktock
Copy link
Member

ktock commented Apr 5, 2021

Also, please squash the commit.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 5, 2021

Build succeeded.

@haslersn
Copy link
Contributor Author

haslersn commented Apr 5, 2021

Also, please squash the commit.

@ktock: Done.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 5, 2021

Build succeeded.

@dmcgowan dmcgowan mentioned this pull request Apr 5, 2021
Closed
mikebrow
mikebrow approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM. just had a couple nit questions on the all 404 and no host found cases.

@ktock
Copy link
Member

ktock commented Apr 14, 2021

@haslersn Please rebase this PR on the latest master. This will solve the CI flakiness.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 14, 2021

Build succeeded.

@ktock
Copy link
Member

ktock commented Apr 14, 2021

@dmcgowan PTAL

@haslersn
Try next mirror in case of non-404 errors, too
c0e2f4b 
Signed-off-by: Sebastian Hasler <sebastian.hasler@gmx.net>
@haslersn
Copy link
Contributor Author

haslersn commented Apr 15, 2021

I had to manually rebase again because of merge conflict.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 15, 2021

Build succeeded.

kzys
kzys approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

LGTM!

/ok-to-test

@haslersn
Copy link
Contributor Author

haslersn commented May 12, 2021

Can this get merged?

ktock
ktock approved these changes May 13, 2021
View reviewed changes
Copy link
Member

@ktock ktock left a comment

Choose a reason for hiding this comment

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

LGTM

@funkypenguin
Copy link

funkypenguin commented May 19, 2021

Hey gang! We're hanging out waiting with bated breath for this one - it'll solve some significant performance issues for us. I'd appreciate it someone with the right access would merge it - looks as if all the pre-requirements have been met... :)

@estesp estesp requested a review from dmcgowan May 19, 2021 14:53
@dmcgowan dmcgowan merged commit dbccd0d into containerd:master May 19, 2021
@ggilley
Copy link

ggilley commented Jul 13, 2021

@dmcgowan How do changes end up in a release? I was hoping this would be in 1.5.3, but it didn't make it...

@JrCs
Copy link

JrCs commented Jul 14, 2021

I have check the code and the merge was NOT include in v1.5.3 👎

@haslersn haslersn deleted the fix-4531 branch July 14, 2021 06:57
@fuweid fuweid mentioned this pull request Jul 16, 2021
Merged
@fuweid
Copy link
Member

fuweid commented Jul 16, 2021

@ggilley @JrCs filed pr to release/1.5 and please wait for next point release.

@thaJeztah thaJeztah added cherry-picked/1.5.x PR commits are cherry-picked into release/1.5 branch and removed cherry-pick/1.5.x labels Jul 20, 2021
@clifford-sanders
Copy link

clifford-sanders commented Nov 11, 2021

@fuweid We also ran into this issue with Artifactory as our pull through cache. Because we use kOps 1.20 we have to use containerd v1.4.x.
Would it be possible to backport the fix to the 1.4.x branch? Thanks

@fuweid fuweid mentioned this pull request Nov 14, 2021
Merged
@fuweid
Copy link
Member

fuweid commented Nov 14, 2021

@clifford-sanders Yes. It is possible. I filed the pr #6244. Please wait for next point release. Thanks

@fuweid fuweid added cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch and removed cherry-pick/1.4.x labels Nov 16, 2021
@jedevc jedevc mentioned this pull request Jul 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikebrow mikebrow mikebrow approved these changes

@kzys kzys kzys approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

@ktock ktock ktock approved these changes

Assignees

No one assigned

Labels

cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch cherry-picked/1.5.x PR commits are cherry-picked into release/1.5 branch ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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