client: enable HTTP(S) keep-alive

[giuseppe]

Enable HTTP(S) keep-alive to improve network performance and reduce latency. We need several HTTP(S) requests before we get to request the blob and each of them requires a new HTTP(S) connection and that slows down significantly pulling images, especially on networks with a higher latency (e.g. wifi).
This will allow multiple requests to be sent over a single connection, reducing the overhead of establishing new connections for each request.

[mtrmac]

I would expect at least the

	IdleConnTimeout:       90 * time.Second,

setting from http.DefaultTransport to be relevant. (Arguably we should just use http.DefaultTransport instead of customizing, but that would be a bigger change than this PR aims for, notably enabling HTTP/2.)

---

Currently, we use many short-lived http.Transport and http.Client objects (basically one per accessed image).

In addition, calling http.Client.CloseIdleConnections after the client is done (e.g. when closing the ImageSource/ImageDestination seems appropriate; idle connections created by one http.Transport can’t be reused by a separate http.Transport AFAICS from reading the implementation.

(I would expect that eventually the server times out and closes the connection, so even without these two things, there should be no long-term leak — but both the server and the client are probably tying up more resources than necessary.)

[mtrmac]

(To be clear, I am in favor of doing this, in general. Thanks for taking the time!)

[mtrmac]

Testing something like this, I still see one connection kept open until the IdleConnTimeout… I’m not sure why.

The need to close docker/client.Client is a great catch.

[mtrmac]

My test patch on top of https://github.com/containers/skopeo/pull/1928

diff --git a/cmd/skopeo/inspect.go b/cmd/skopeo/inspect.go
index e969caa4..0e02959a 100644
--- a/cmd/skopeo/inspect.go
+++ b/cmd/skopeo/inspect.go
@@ -1,11 +1,16 @@
 package main
 
 import (
+       "bytes"
        "encoding/json"
        "errors"
        "fmt"
        "io"
+       "os"
+       "os/exec"
+       "regexp"
        "strings"
+       "time"
 
        "github.com/containers/common/pkg/report"
        "github.com/containers/common/pkg/retry"
@@ -100,11 +105,11 @@ func (opts *inspectOptions) run(args []string, stdout io.Writer) (retErr error)
                return fmt.Errorf("Error parsing image name %q: %w", imageName, err)
        }
 
-       defer func() {
-               if err := src.Close(); err != nil {
-                       retErr = noteCloseFailure(retErr, "closing image", err)
-               }
-       }()
+       // defer func() {
+       //      if err := src.Close(); err != nil {
+       //              retErr = noteCloseFailure(retErr, "closing image", err)
+       //      }
+       // }()
 
        if err := retry.IfNecessary(ctx, func() error {
                rawManifest, _, err = src.GetManifest(ctx, nil)
@@ -214,7 +219,49 @@ func (opts *inspectOptions) run(args []string, stdout io.Writer) (retErr error)
                        logrus.Warnf("Registry disallows tag list retrieval; skipping")
                }
        }
-       return opts.writeOutput(stdout, outputData)
+       if err := opts.writeOutput(stdout, outputData); err != nil {
+               return err
+       }
+       if err := src.Close(); err != nil {
+               return err
+       }
+
+       start := time.Now()
+       last := ""
+       myPID := fmt.Sprintf("%d", os.Getpid())
+       for i := 0; i < 300; i++ {
+               out, err := exec.Command("lsof", "-p", myPID).CombinedOutput()
+               if err != nil {
+                       return err
+               }
+               cleaned := clean(out)
+               if last != cleaned {
+                       fmt.Fprintf(stdout, "==========@ %f\n%s\n", float64(time.Since(start).Milliseconds())/1000.0, cleaned)
+                       last = cleaned
+               }
+               time.Sleep(1 * time.Second)
+       }
+       return nil
+}
+
+var cleanPairs = []struct {
+       regexp      *regexp.Regexp
+       replacement []byte
+}{
+       {regexp.MustCompile("PIPE[ \t]+"), []byte("PIPE ")},  // Account for PIPE entry alignment differences
+       {regexp.MustCompile("0x[0-9a-f]+"), []byte("0xHEX")}, // Various file IDs
+       {regexp.MustCompile("0t[0-9]+"), []byte("0tTTY")},    // TTY offset
+}
+
+func clean(in []byte) string {
+       b := strings.Builder{}
+       for _, line := range bytes.SplitAfter(in, []byte{'\n'}) {
+               for _, pair := range cleanPairs {
+                       line = pair.regexp.ReplaceAll(line, pair.replacement)
+               }
+               b.Write(line)
+       }
+       return b.String()
 }
 
 // writeOutput writes data depending on opts.format to stdout
diff --git a/vendor/github.com/containers/image/v5/docker/docker_image.go b/vendor/github.com/containers/image/v5/docker/docker_image.go
index 6e121533..bb81b24b 100644
--- a/vendor/github.com/containers/image/v5/docker/docker_image.go
+++ b/vendor/github.com/containers/image/v5/docker/docker_image.go
@@ -14,6 +14,7 @@ import (
        "github.com/containers/image/v5/manifest"
        "github.com/containers/image/v5/types"
        "github.com/opencontainers/go-digest"
+       "github.com/sirupsen/logrus"
 )
 
 // Image is a Docker-specific implementation of types.ImageCloser with a few extra methods
@@ -68,6 +69,11 @@ func GetRepositoryTags(ctx context.Context, sys *types.SystemContext, ref types.
        if err != nil {
                return nil, fmt.Errorf("failed to create client: %w", err)
        }
+       defer func() {
+               logrus.Errorf("client=%#v", client)
+               logrus.Errorf("client.client=%#v", client.client)
+               client.client.CloseIdleConnections()
+       }()
 
        tags := make([]string, 0)
 
diff --git a/vendor/github.com/containers/image/v5/docker/docker_image_src.go b/vendor/github.com/containers/image/v5/docker/docker_image_src.go
index a115268d..0b193be4 100644
--- a/vendor/github.com/containers/image/v5/docker/docker_image_src.go
+++ b/vendor/github.com/containers/image/v5/docker/docker_image_src.go
@@ -166,6 +166,7 @@ func (s *dockerImageSource) Reference() types.ImageReference {
 
 // Close removes resources associated with an initialized ImageSource, if any.
 func (s *dockerImageSource) Close() error {
+       s.c.client.CloseIdleConnections()
        return nil
 }
 
diff --git a/vendor/github.com/containers/image/v5/pkg/tlsclientconfig/tlsclientconfig.go b/vendor/github.com/containers/image/v5/pkg/tlsclientconfig/tlsclientconfig.go
index 96af5ace..106fc14c 100644
--- a/vendor/github.com/containers/image/v5/pkg/tlsclientconfig/tlsclientconfig.go
+++ b/vendor/github.com/containers/image/v5/pkg/tlsclientconfig/tlsclientconfig.go
@@ -97,8 +97,10 @@ func NewTransport() *http.Transport {
                Proxy:               http.ProxyFromEnvironment,
                DialContext:         direct.DialContext,
                TLSHandshakeTimeout: 10 * time.Second,
+               MaxIdleConns:        100,
+               IdleConnTimeout:     90 * time.Second,
                // TODO(dmcgowan): Call close idle connections when complete and use keep alive
-               DisableKeepAlives: true,
+               // DisableKeepAlives: true,
        }
        return tr
 }

[mtrmac]

Testing something like this, I still see one connection kept open until the IdleConnTimeout… I’m not sure why.

Turns out I forgot to patch GetRepositoryTags to close its client. With that, all sockets are closed immediately after inspect is done.

[giuseppe]

@mtrmac thanks for you review. I've addressed your comments and pushed a new version

[mtrmac]

Thanks!

LGTM.

[giuseppe]

@vrothberg PTAL

[vrothberg]

Nice!
LGTM