Skip to content
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.

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

docker transport: catch disconnects #1295

Open
vrothberg opened this issue Jul 8, 2021 · 4 comments
Open

docker transport: catch disconnects #1295

vrothberg opened this issue Jul 8, 2021 · 4 comments
Labels
kind/feature A request for, or a PR adding, new functionality

Comments

@vrothberg
Copy link
Member

https://bugzilla.redhat.com/show_bug.cgi?id=1898309 is asking to add some more information to users when loosing the connection to/from a registry. I'll move the technical discussion here.

Skimming the code, we may need to ping the registry when facing an error. (c *dockerClient) detectPropertiesHelper(...) may be a way to do that:

func PutBlob(...) {
  if err != nil {
    if err := detectPropertiesHelper(); err != nil {
         logrus.Debug("Probably lost connection to the container registry")
   } [..]

@mtrmac @lsm5 WDYT?

@vrothberg vrothberg changed the title docker transport: catch disconects docker transport: catch disconnects Jul 8, 2021
@mtrmac
Copy link
Collaborator

mtrmac commented Jul 8, 2021

  • I’m overall still dissatisfied with our understanding of the root cause so far. io.EOF should be a termination condition, never a user-reported failure, so it’s puzzling how it shows up in an error message. Compare [FEAT] Catch EOF on skopeo copy when one side or the other disconnects and log more information. #1083 and http.Transport configuration enforces http1.1 and causes issues with istio #1139 . Do we know what is happening well enough to add a heuristic?
    • Would we say “probably lost connection” on a failure to contact /v2/ or on success? I mean, if we lose internet connection completely and there is a timeout, the original error probably stands well enough on its own. If the hypothesis is that something in the network times out connection tracking and sinkholes the connection, the following connection to /v2/ is going to succeed, not fail!
  • (To a little extent, we are already adding context: see copy.errorAnnotationReader. That’s not a substitute for such a heuristic, sure.)
  • Using the /v2/ URL to check connectivity makes sense, but I don’t think we should do all of detectPropertiesHelper
    • See how dockerClient fields are documented to never change after detectProperties
    • We already know that the registry is v2, and whether to use HTTP or HTTPS; we don’t need to try all of the endpoints again.

@mtrmac
Copy link
Collaborator

mtrmac commented Jul 8, 2021

  • I’m overall still dissatisfied with our understanding of the root cause so far.

Note that the old reports, pre-#1073 , don’t even make it clear whether it is the source or the destination failing!

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 21, 2022

For the record, one possible path where the net/http stack can return io.EOF and not an “unexpected EOF” is if the server reads the request, and cleanly closes the connection without producing any response header. Reported as golang/go#53472 .

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 9, 2022
@mtrmac
Copy link
Collaborator

mtrmac commented Sep 20, 2024

#1816 has added a “connection reset” retry logic to reads from registries. Note that it doesn’t handle io.EOF (which should not happen per API rules) nor url.Error{io.EOF} (per the previous comment).

The original report is for writes, though. In that case we can’t retry (because we are streaming the input and we no longer have a copy after a failure).

We could add some heuristic error message context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

No branches or pull requests

2 participants