Hi @teemuteemu. 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.

Build succeeded.

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

/ok-to-test

More error handling logic needs to be done here to if expanding the status code which are handled. The logic of the for loop assumes that lastErr == nil at the end meant that either no registries were given or each registry returned a 404. For non-404 cases, lastErr should at least be set and used to indicate the first unexpected registry response.

I see, I'll take a better look at the error handling.

@teemuteemu any news ?

@teemuteemu @dmcgowan How about something like:

			if resp.StatusCode > 299 {
				// in case of error try next host
				if resp.StatusCode > 399 {
					if resp.StatusCode != http.StatusNotFound {
						lastErr = errors.Errorf("unexpected status code %v: %v", u, resp.Status)
					}
					continue
				}
				return "", ocispec.Descriptor{}, errors.Errorf("unexpected status code %v: %v", u, resp.Status)
			}

@teemuteemu @dmcgowan How about something like:

			if resp.StatusCode > 299 {
				// in case of error try next host
				if resp.StatusCode > 399 {
					if resp.StatusCode != http.StatusNotFound {
						lastErr = errors.Errorf("unexpected status code %v: %v", u, resp.Status)
					}
					continue
				}
				return "", ocispec.Descriptor{}, errors.Errorf("unexpected status code %v: %v", u, resp.Status)
			}

We run this patch in production for a few weeks now and it works great without any issues. We run Kraken as a p2p caching layer for image pulls within clusters that sometimes has some hiccups. This patch allows a much more reliant failover in these cases to directly contact our internal registry.

@teemuteemu Can you include the error handling changes in your PR? I would love to see this change in upstream containerd and stop maintaining the patch for new releases.

@mariuskiessling Maybe you should create a new PR since @teemuteemu doesn't seem to be responding. And you have proof of it working :-)

@ggilley I would but it seems like that @haslersn has already done that. Thus, I am for closing this PR in favour of #5275.

agreed, closing in favor of #5275