[image-builder] Introduce retry for all resolver requests (timeout 15s, up to 3 times) for all network-related issues (feature flag: imagebuilder_retry_resolve)

Author: geropl

components/image-builder-mk3/cmd/run.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/gitpod-io/gitpod/common-go/baseserver"
+	"github.com/gitpod-io/gitpod/common-go/experiments"
 
 	"github.com/gitpod-io/gitpod/common-go/log"
 	"github.com/gitpod-io/gitpod/image-builder/api"
@@ -40,7 +41,12 @@ var runCmd = &cobra.Command{
 		span, ctx := opentracing.StartSpanFromContext(ctx, "/cmd/Run")
 		defer span.Finish()
 
-		service, err := orchestrator.NewOrchestratingBuilder(cfg.Orchestrator)
+		// Check feature flag for retry client
+		experimentsClient := experiments.NewClient()
+		useRetryClient := experimentsClient.GetBoolValue(ctx, "imagebuilder_retry_resolve", false, experiments.Attributes{})
+		log.WithField("useRetryClient", useRetryClient).Info("Feature flag imagebuilder_retry_resolve checked")
+
+		service, err := orchestrator.NewOrchestratingBuilder(cfg.Orchestrator, useRetryClient)
 		if err != nil {
 			log.Fatal(err)
 		}

components/image-builder-mk3/go.mod

@@ -93,6 +93,8 @@ require (
 
 require (
 	github.com/Microsoft/hcsshim v0.11.4 // indirect
+	github.com/blang/semver v3.5.1+incompatible // indirect
+	github.com/configcat/go-sdk/v7 v7.6.0 // indirect
 	github.com/containerd/errdefs v0.1.0 // indirect
 	github.com/containerd/log v0.1.0 // indirect
 	github.com/jmespath/go-jmespath v0.4.0 // indirect

components/image-builder-mk3/go.sum

@@ -56,7 +56,7 @@ const (
 )
 
 // NewOrchestratingBuilder creates a new orchestrating image builder
-func NewOrchestratingBuilder(cfg config.Configuration) (res *Orchestrator, err error) {
+func NewOrchestratingBuilder(cfg config.Configuration, useRetryClient bool) (res *Orchestrator, err error) {
 	var authentication auth.CompositeAuth
 	if cfg.PullSecretFile != "" {
 		fn := cfg.PullSecretFile
@@ -106,14 +106,26 @@ func NewOrchestratingBuilder(cfg config.Configuration) (res *Orchestrator, err e
 		wsman = wsmanapi.NewWorkspaceManagerClient(conn)
 	}
 
+	// Create RefResolver with conditional retry client based on feature flag
+	var refResolver resolve.DockerRefResolver
+	if useRetryClient {
+		refResolver = &resolve.StandaloneRefResolver{
+			Client: NewRetryTimeoutClient(),
+		}
+		log.Info("Using retry timeout client for Docker registry requests")
+	} else {
+		refResolver = &resolve.StandaloneRefResolver{}
+		log.Info("Using standard client for Docker registry requests")
+	}
+
 	o := &Orchestrator{
 		Config: cfg,
 		Auth:   authentication,
 		AuthResolver: auth.Resolver{
 			BaseImageRepository:      cfg.BaseImageRepository,
 			WorkspaceImageRepository: cfg.WorkspaceImageRepository,
 		},
-		RefResolver: &resolve.StandaloneRefResolver{},
+		RefResolver: refResolver,
 
 		wsman:         wsman,
 		buildListener: make(map[string]map[buildListener]struct{}),

components/image-builder-mk3/pkg/orchestrator/orchestrator.go

@@ -138,7 +138,7 @@ func TestBuild(t *testing.T) {
 				BaseImageRepository:      "registry/base",
 				WorkspaceImageRepository: "registry/workspace",
 				BuilderImage:             "builder-image",
-			})
+			}, false)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -188,7 +188,7 @@ func TestResolveBaseImage(t *testing.T) {
 				WorkspaceManager: config.WorkspaceManagerConfig{
 					Client: wsmock.NewMockWorkspaceManagerClient(ctrl),
 				},
-			})
+			}, false)
 			if err != nil {
 				t.Fatal(err)
 			}

components/image-builder-mk3/pkg/orchestrator/orchestrator_test.go

@@ -0,0 +1,126 @@
+// Copyright (c) 2021 Gitpod GmbH. All rights reserved.
+// Licensed under the GNU Affero General Public License (AGPL).
+// See License.AGPL.txt in the project root for license information.
+
+package orchestrator
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"net"
+	"net/http"
+	"strings"
+	"time"
+
+	"github.com/gitpod-io/gitpod/common-go/log"
+)
+
+const (
+	// refResolverTimeout is the timeout for individual HTTP requests to Docker registries
+	refResolverTimeout = 15 * time.Second
+
+	// refResolverMaxRetries is the maximum number of retry attempts for registry requests
+	refResolverMaxRetries = 3
+)
+
+// RetryRoundTripper implements http.RoundTripper with retry logic
+// for Docker registry requests. It retries only on timeouts and network connectivity issues.
+type RetryRoundTripper struct {
+	base       http.RoundTripper
+	maxRetries int
+}
+
+// NewRetryTimeoutClient creates a new HTTP client with timeout and retry capabilities
+func NewRetryTimeoutClient() *http.Client {
+	retryTransport := &RetryRoundTripper{
+		base:       http.DefaultTransport,
+		maxRetries: refResolverMaxRetries,
+	}
+
+	return &http.Client{
+		Timeout:   refResolverTimeout,
+		Transport: retryTransport,
+	}
+}
+
+// RoundTrip executes an HTTP request with retry logic for timeouts and network errors
+func (r *RetryRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	var lastResp *http.Response
+	var lastErr error
+
+	// Use the existing retry function from monitor.go
+	err := retry(req.Context(), func(ctx context.Context) error {
+		// Clone the request for each retry attempt
+		reqClone := req.Clone(ctx)
+		resp, err := r.base.RoundTrip(reqClone)
+		if err != nil {
+			lastErr = err
+			lastResp = nil
+			return err
+		}
+		// Store the response for successful requests
+		lastResp = resp
+		lastErr = nil
+		return nil
+	}, r.shouldRetry, 1*time.Second, r.maxRetries)
+
+	if err != nil {
+		if errors.Is(err, errOutOfRetries) {
+			// Return the last error from the actual HTTP request
+			if lastErr != nil {
+				return nil, lastErr
+			}
+			return nil, fmt.Errorf("registry request failed after %d retries", r.maxRetries)
+		}
+		return nil, err
+	}
+
+	// If we get here, the last attempt was successful
+	return lastResp, nil
+}
+
+// shouldRetry determines if an error should trigger a retry attempt
+// Only retries on timeouts and network connectivity issues
+func (r *RetryRoundTripper) shouldRetry(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Check for timeout errors
+	if errors.Is(err, context.DeadlineExceeded) {
+		log.WithError(err).Debug("retrying registry request due to timeout")
+		return true
+	}
+
+	// Check for network connectivity issues
+	if r.isNetworkError(err) {
+		log.WithError(err).Debug("retrying registry request due to network error")
+		return true
+	}
+
+	return false
+}
+
+// isNetworkError checks if an error is a network connectivity issue
+func (r *RetryRoundTripper) isNetworkError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Check for net.Error interface (includes timeout, temporary errors)
+	if netErr, ok := err.(net.Error); ok {
+		if netErr.Timeout() || netErr.Temporary() {
+			return true
+		}
+	}
+
+	// Check for common network error strings
+	errorStr := err.Error()
+	return strings.Contains(errorStr, "connection refused") ||
+		strings.Contains(errorStr, "no such host") ||
+		strings.Contains(errorStr, "network is unreachable") ||
+		strings.Contains(errorStr, "connection reset by peer") ||
+		strings.Contains(errorStr, "i/o timeout") ||
+		strings.Contains(errorStr, "net/http: TLS handshake timeout")
+}

components/image-builder-mk3/pkg/orchestrator/retryclient.go

@@ -0,0 +1,130 @@
+// Copyright (c) 2021 Gitpod GmbH. All rights reserved.
+// Licensed under the GNU Affero General Public License (AGPL).
+// See License.AGPL.txt in the project root for license information.
+
+package orchestrator
+
+import (
+	"errors"
+	"net"
+	"net/http"
+	"sync/atomic"
+	"testing"
+)
+
+// TestRetryRoundTripper tests the retry logic for network errors and timeouts
+func TestRetryRoundTripper(t *testing.T) {
+	tests := []struct {
+		name            string
+		errorType       string
+		expectedRetries int
+		shouldSucceed   bool
+	}{
+		{
+			name:            "timeout error should retry",
+			errorType:       "timeout",
+			expectedRetries: 3,
+			shouldSucceed:   false,
+		},
+		{
+			name:            "connection refused should retry",
+			errorType:       "connection_refused",
+			expectedRetries: 3,
+			shouldSucceed:   false,
+		},
+		{
+			name:            "network unreachable should retry",
+			errorType:       "network_unreachable",
+			expectedRetries: 3,
+			shouldSucceed:   false,
+		},
+		{
+			name:            "success after retries",
+			errorType:       "success_after_retry",
+			expectedRetries: 2,
+			shouldSucceed:   true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var attemptCount int64
+
+			// Create a mock transport that simulates different error conditions
+			mockTransport := &mockRoundTripper{
+				errorType:    tt.errorType,
+				attemptCount: &attemptCount,
+				maxAttempts:  int64(tt.expectedRetries),
+			}
+
+			retryTransport := &RetryRoundTripper{
+				base:       mockTransport,
+				maxRetries: refResolverMaxRetries,
+			}
+
+			req, err := http.NewRequest("GET", "http://example.com", nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			_, err = retryTransport.RoundTrip(req)
+
+			if tt.shouldSucceed && err != nil {
+				t.Errorf("Expected success but got error: %v", err)
+			}
+			if !tt.shouldSucceed && err == nil {
+				t.Errorf("Expected error but got success")
+			}
+
+			actualAttempts := atomic.LoadInt64(&attemptCount)
+			if actualAttempts != int64(tt.expectedRetries) {
+				t.Errorf("Expected %d attempts, got %d", tt.expectedRetries, actualAttempts)
+			}
+		})
+	}
+}
+
+// mockRoundTripper simulates different network error conditions for testing
+type mockRoundTripper struct {
+	errorType    string
+	attemptCount *int64
+	maxAttempts  int64
+}
+
+func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	attempt := atomic.AddInt64(m.attemptCount, 1)
+
+	switch m.errorType {
+	case "timeout":
+		return nil, &net.OpError{
+			Op:  "dial",
+			Net: "tcp",
+			Err: &timeoutError{},
+		}
+	case "connection_refused":
+		return nil, errors.New("connection refused")
+	case "network_unreachable":
+		return nil, errors.New("network is unreachable")
+	case "success_after_retry":
+		if attempt < m.maxAttempts {
+			return nil, errors.New("connection refused")
+		}
+		// Success on the last attempt
+		return &http.Response{
+			StatusCode: 200,
+			Body:       http.NoBody,
+		}, nil
+	default:
+		return &http.Response{
+			StatusCode: 200,
+			Body:       http.NoBody,
+		}, nil
+	}
+}
+
+// timeoutError implements net.Error with Timeout() returning true
+type timeoutError struct{}
+
+func (e *timeoutError) Error() string   { return "timeout" }
+func (e *timeoutError) Timeout() bool   { return true }
+func (e *timeoutError) Temporary() bool { return false }