From c3691cded2d9a656db355d5931d313d16d3f928f Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 27 Feb 2023 10:09:51 +0100 Subject: [PATCH] client: enable HTTP(S) keep-alive 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. Signed-off-by: Giuseppe Scrivano --- docker/daemon/client_test.go | 5 +++++ docker/daemon/daemon_dest.go | 1 + docker/daemon/daemon_src.go | 2 ++ docker/docker_client.go | 10 ++++++++++ docker/docker_image.go | 2 ++ docker/docker_image_dest.go | 2 +- docker/docker_image_src.go | 4 +++- oci/layout/oci_src.go | 1 + pkg/tlsclientconfig/tlsclientconfig.go | 4 ++-- 9 files changed, 27 insertions(+), 4 deletions(-) diff --git a/docker/daemon/client_test.go b/docker/daemon/client_test.go index 2cc12e24c0..161747f750 100644 --- a/docker/daemon/client_test.go +++ b/docker/daemon/client_test.go @@ -19,6 +19,8 @@ func TestDockerClientFromNilSystemContext(t *testing.T) { assert.Equal(t, dockerclient.DefaultDockerHost, client.DaemonHost(), "The default docker host should have been used") assert.Equal(t, defaultAPIVersion, client.ClientVersion(), "The default api version should have been used") + + assert.NoError(t, client.Close()) } func TestDockerClientFromCertContext(t *testing.T) { @@ -38,6 +40,8 @@ func TestDockerClientFromCertContext(t *testing.T) { assert.Equal(t, host, client.DaemonHost()) assert.Equal(t, "1.22", client.ClientVersion()) + + assert.NoError(t, client.Close()) } func TestTlsConfigFromInvalidCertPath(t *testing.T) { @@ -94,6 +98,7 @@ func TestSpecifyPlainHTTPViaHostScheme(t *testing.T) { assert.NotNil(t, client, "A Docker client reference should have been returned") assert.Equal(t, host, client.DaemonHost()) + assert.NoError(t, client.Close()) } func testDir(t *testing.T) string { diff --git a/docker/daemon/daemon_dest.go b/docker/daemon/daemon_dest.go index dc4aa70d3a..59e02462f0 100644 --- a/docker/daemon/daemon_dest.go +++ b/docker/daemon/daemon_dest.go @@ -69,6 +69,7 @@ func newImageDestination(ctx context.Context, sys *types.SystemContext, ref daem // imageLoadGoroutine accepts tar stream on reader, sends it to c, and reports error or success by writing to statusChannel func imageLoadGoroutine(ctx context.Context, c *client.Client, reader *io.PipeReader, statusChannel chan<- error) { + defer c.Close() err := errors.New("Internal error: unexpected panic in imageLoadGoroutine") defer func() { logrus.Debugf("docker-daemon: sending done, status %v", err) diff --git a/docker/daemon/daemon_src.go b/docker/daemon/daemon_src.go index b57936654b..10923c278e 100644 --- a/docker/daemon/daemon_src.go +++ b/docker/daemon/daemon_src.go @@ -28,6 +28,8 @@ func newImageSource(ctx context.Context, sys *types.SystemContext, ref daemonRef if err != nil { return nil, fmt.Errorf("initializing docker engine client: %w", err) } + defer c.Close() + // Per NewReference(), ref.StringWithinTransport() is either an image ID (config digest), or a !reference.NameOnly() reference. // Either way ImageSave should create a tarball with exactly one image. inputStream, err := c.ImageSave(ctx, []string{ref.StringWithinTransport()}) diff --git a/docker/docker_client.go b/docker/docker_client.go index fa749375fb..04e4e474c6 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -213,6 +213,7 @@ func dockerCertDir(sys *types.SystemContext, hostPort string) (string, error) { // newDockerClientFromRef returns a new dockerClient instance for refHostname (a host a specified in the Docker image reference, not canonicalized to dockerRegistry) // “write” specifies whether the client will be used for "write" access (in particular passed to lookaside.go:toplevelFromSection) // signatureBase is always set in the return value +// The caller must call .Close() on the returned client when done. func newDockerClientFromRef(sys *types.SystemContext, ref dockerReference, registryConfig *registryConfiguration, write bool, actions string) (*dockerClient, error) { auth, err := config.GetCredentialsForRef(sys, ref.ref) if err != nil { @@ -247,6 +248,7 @@ func newDockerClientFromRef(sys *types.SystemContext, ref dockerReference, regis // (e.g., "registry.com[:5000][/some/namespace]/repo"). // Please note that newDockerClient does not set all members of dockerClient // (e.g., username and password); those must be set by callers if necessary. +// The caller must call .Close() on the returned client when done. func newDockerClient(sys *types.SystemContext, registry, reference string) (*dockerClient, error) { hostName := registry if registry == dockerHostname { @@ -302,6 +304,7 @@ func CheckAuth(ctx context.Context, sys *types.SystemContext, username, password if err != nil { return fmt.Errorf("creating new docker client: %w", err) } + defer client.Close() client.auth = types.DockerAuthConfig{ Username: username, Password: password, @@ -371,6 +374,7 @@ func SearchRegistry(ctx context.Context, sys *types.SystemContext, registry, ima if err != nil { return nil, fmt.Errorf("creating new docker client: %w", err) } + defer client.Close() client.auth = auth if sys != nil { client.registryToken = sys.DockerBearerRegistryToken @@ -1084,3 +1088,9 @@ func (c *dockerClient) getExtensionsSignatures(ctx context.Context, ref dockerRe func sigstoreAttachmentTag(d digest.Digest) string { return strings.Replace(d.String(), ":", "-", 1) + ".sig" } + +// Close removes resources associated with an initialized dockerClient, if any. +func (c *dockerClient) Close() error { + c.client.CloseIdleConnections() + return nil +} diff --git a/docker/docker_image.go b/docker/docker_image.go index 6e121533ed..42bbfd95ee 100644 --- a/docker/docker_image.go +++ b/docker/docker_image.go @@ -68,6 +68,7 @@ func GetRepositoryTags(ctx context.Context, sys *types.SystemContext, ref types. if err != nil { return nil, fmt.Errorf("failed to create client: %w", err) } + defer client.Close() tags := make([]string, 0) @@ -136,6 +137,7 @@ func GetDigest(ctx context.Context, sys *types.SystemContext, ref types.ImageRef if err != nil { return "", fmt.Errorf("failed to create client: %w", err) } + defer client.Close() path := fmt.Sprintf(manifestPath, reference.Path(dr.ref), tagOrDigest) headers := map[string][]string{ diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index 9652683852..78c81a3df2 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -93,7 +93,7 @@ func (d *dockerImageDestination) Reference() types.ImageReference { // Close removes resources associated with an initialized ImageDestination, if any. func (d *dockerImageDestination) Close() error { - return nil + return d.c.Close() } // SupportsSignatures returns an error (to be displayed to the user) if the destination certainly can't store signatures. diff --git a/docker/docker_image_src.go b/docker/docker_image_src.go index a115268de3..231d5d2124 100644 --- a/docker/docker_image_src.go +++ b/docker/docker_image_src.go @@ -153,6 +153,7 @@ func newImageSourceAttempt(ctx context.Context, sys *types.SystemContext, logica s.Compat = impl.AddCompat(s) if err := s.ensureManifestIsLoaded(ctx); err != nil { + client.Close() return nil, err } return s, nil @@ -166,7 +167,7 @@ func (s *dockerImageSource) Reference() types.ImageReference { // Close removes resources associated with an initialized ImageSource, if any. func (s *dockerImageSource) Close() error { - return nil + return s.c.Close() } // simplifyContentType drops parameters from a HTTP media type (see https://tools.ietf.org/html/rfc7231#section-3.1.1.1) @@ -605,6 +606,7 @@ func deleteImage(ctx context.Context, sys *types.SystemContext, ref dockerRefere if err != nil { return err } + defer c.Close() headers := map[string][]string{ "Accept": manifest.DefaultRequestedManifestMIMETypes, diff --git a/oci/layout/oci_src.go b/oci/layout/oci_src.go index 817a4e40d0..6b423f3b05 100644 --- a/oci/layout/oci_src.go +++ b/oci/layout/oci_src.go @@ -94,6 +94,7 @@ func (s *ociImageSource) Reference() types.ImageReference { // Close removes resources associated with an initialized ImageSource, if any. func (s *ociImageSource) Close() error { + s.client.CloseIdleConnections() return nil } diff --git a/pkg/tlsclientconfig/tlsclientconfig.go b/pkg/tlsclientconfig/tlsclientconfig.go index 5301f192a9..56b0d49390 100644 --- a/pkg/tlsclientconfig/tlsclientconfig.go +++ b/pkg/tlsclientconfig/tlsclientconfig.go @@ -96,8 +96,8 @@ func NewTransport() *http.Transport { Proxy: http.ProxyFromEnvironment, DialContext: direct.DialContext, TLSHandshakeTimeout: 10 * time.Second, - // TODO(dmcgowan): Call close idle connections when complete and use keep alive - DisableKeepAlives: true, + IdleConnTimeout: 90 * time.Second, + MaxIdleConns: 100, } return tr }