Merge pull request #13380 from guangxuli/fix_docker_pull_fail

Merged by openshift-bot

Author: OpenShift Bot

pkg/build/builder/dockerutil.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
+	"reflect"
 	"strings"
 	"time"
 
@@ -21,11 +22,11 @@ import (
 )
 
 var (
-	// DefaultPushRetryCount is the number of retries of pushing the built Docker image
+	// DefaultPushOrPullRetryCount is the number of retries of pushing or pulling the built Docker image
 	// into a configured repository
-	DefaultPushRetryCount = 6
-	// DefaultPushRetryDelay is the time to wait before triggering a push retry
-	DefaultPushRetryDelay = 5 * time.Second
+	DefaultPushOrPullRetryCount = 6
+	// DefaultPushOrPullRetryDelay is the time to wait before triggering a push or pull retry
+	DefaultPushOrPullRetryDelay = 5 * time.Second
 	// RetriableErrors is a set of strings that indicate that an retriable error occurred.
 	RetriableErrors = []string{
 		"ping attempt failed with error",
@@ -54,6 +55,47 @@ type DockerClient interface {
 	TagImage(name string, opts docker.TagImageOptions) error
 }
 
+func RetryImageAction(client DockerClient, opts interface{}, authConfig docker.AuthConfiguration) error {
+	var err error
+	var retriableError = false
+	var actionName string
+
+	pullOpt := docker.PullImageOptions{}
+	pushOpt := docker.PushImageOptions{}
+
+	for retries := 0; retries <= DefaultPushOrPullRetryCount; retries++ {
+		if reflect.TypeOf(opts) == reflect.TypeOf(pullOpt) {
+			actionName = "Pull"
+			err = client.PullImage(opts.(docker.PullImageOptions), authConfig)
+		} else if reflect.TypeOf(opts) == reflect.TypeOf(pushOpt) {
+			actionName = "Push"
+			err = client.PushImage(opts.(docker.PushImageOptions), authConfig)
+		} else {
+			return errors.New("not match Pull or Push action")
+		}
+
+		if err == nil {
+			return nil
+		}
+
+		errMsg := fmt.Sprintf("%s", err)
+		for _, errorString := range RetriableErrors {
+			if strings.Contains(errMsg, errorString) {
+				retriableError = true
+				break
+			}
+		}
+		if !retriableError {
+			return err
+		}
+
+		glog.V(0).Infof("Warning: %s failed, retrying in %s ...", actionName, DefaultPushOrPullRetryDelay)
+		time.Sleep(DefaultPushOrPullRetryDelay)
+	}
+
+	return fmt.Errorf("After retrying %d times, %s image still failed", DefaultPushOrPullRetryDelay, actionName)
+}
+
 func pullImage(client DockerClient, name string, authConfig docker.AuthConfiguration) error {
 	logProgress := func(s string) {
 		glog.V(0).Infof("%s", s)
@@ -67,11 +109,7 @@ func pullImage(client DockerClient, name string, authConfig docker.AuthConfigura
 		opts.OutputStream = os.Stderr
 		opts.RawJSONStream = false
 	}
-	err := client.PullImage(opts, authConfig)
-	if err == nil {
-		return nil
-	}
-	return err
+	return RetryImageAction(client, opts, authConfig)
 }
 
 // pushImage pushes a docker image to the registry specified in its tag.
@@ -102,30 +140,11 @@ func pushImage(client DockerClient, name string, authConfig docker.AuthConfigura
 		OutputStream:  io.MultiWriter(progressWriter, digestWriter),
 		RawJSONStream: true,
 	}
-	var err error
-	var retriableError = false
-
-	for retries := 0; retries <= DefaultPushRetryCount; retries++ {
-		err = client.PushImage(opts, authConfig)
-		if err == nil {
-			return digestWriter.Digest, nil
-		}
-
-		errMsg := fmt.Sprintf("%s", err)
-		for _, errorString := range RetriableErrors {
-			if strings.Contains(errMsg, errorString) {
-				retriableError = true
-				break
-			}
-		}
-		if !retriableError {
-			return "", err
-		}
 
-		glog.V(0).Infof("Warning: Push failed, retrying in %s ...", DefaultPushRetryDelay)
-		time.Sleep(DefaultPushRetryDelay)
+	if err := RetryImageAction(client, opts, authConfig); err != nil {
+		return "", err
 	}
-	return "", err
+	return digestWriter.Digest, nil
 }
 
 func removeImage(client DockerClient, name string) error {

pkg/build/builder/dockerutil_test.go

@@ -17,6 +17,7 @@ import (
 
 type FakeDocker struct {
 	pushImageFunc   func(opts docker.PushImageOptions, auth docker.AuthConfiguration) error
+	pullImageFunc   func(opts docker.PullImageOptions, auth docker.AuthConfiguration) error
 	buildImageFunc  func(opts docker.BuildImageOptions) error
 	removeImageFunc func(name string) error
 
@@ -51,6 +52,20 @@ func fakePushImageFunc(opts docker.PushImageOptions, auth docker.AuthConfigurati
 	}
 	return nil
 }
+
+func fakePullImageFunc(opts docker.PullImageOptions, auth docker.AuthConfiguration) error {
+	switch opts.Repository {
+	case "repo_test_succ_foo_bar":
+		return nil
+	case "repo_test_err_exist_foo_bar":
+		fooBarRunTimes++
+		return errors.New(RetriableErrors[0])
+	case "repo_test_err_no_exist_foo_bar":
+		return errors.New("no_exist_err_foo_bar")
+	}
+	return nil
+}
+
 func (d *FakeDocker) BuildImage(opts docker.BuildImageOptions) error {
 	if d.buildImageFunc != nil {
 		return d.buildImageFunc(opts)
@@ -77,6 +92,9 @@ func (d *FakeDocker) DownloadFromContainer(id string, opts docker.DownloadFromCo
 	return nil
 }
 func (d *FakeDocker) PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error {
+	if d.pullImageFunc != nil {
+		return d.pullImageFunc(opts, auth)
+	}
 	return nil
 }
 func (d *FakeDocker) RemoveContainer(opts docker.RemoveContainerOptions) error {
@@ -141,23 +159,23 @@ func TestTagImage(t *testing.T) {
 func TestPushImage(t *testing.T) {
 	var testImageName string
 
-	bakRetryCount := DefaultPushRetryCount
-	bakRetryDelay := DefaultPushRetryDelay
+	bakRetryCount := DefaultPushOrPullRetryCount
+	bakRetryDelay := DefaultPushOrPullRetryDelay
 
 	fakeDocker := NewFakeDockerClient()
 	fakeDocker.pushImageFunc = fakePushImageFunc
 	testAuth := docker.AuthConfiguration{
-		Username:      "usernname_foo_bar",
+		Username:      "username_foo_bar",
 		Password:      "password_foo_bar",
 		Email:         "email_foo_bar",
 		ServerAddress: "serveraddress_foo_bar",
 	}
 
 	//make test quickly, and recover the value after testing
-	DefaultPushRetryCount = 2
-	defer func() { DefaultPushRetryCount = bakRetryCount }()
-	DefaultPushRetryDelay = 1
-	defer func() { DefaultPushRetryDelay = bakRetryDelay }()
+	DefaultPushOrPullRetryCount = 2
+	defer func() { DefaultPushOrPullRetryCount = bakRetryCount }()
+	DefaultPushOrPullRetryDelay = 1
+	defer func() { DefaultPushOrPullRetryDelay = bakRetryDelay }()
 
 	//expect succ
 	testImageName = "repo_foo_bar:tag_test_succ_foo_bar"
@@ -172,7 +190,7 @@ func TestPushImage(t *testing.T) {
 		t.Errorf("Unexpect push image : %v, want error", err)
 	}
 	//expect run 3 times
-	if fooBarRunTimes != (DefaultPushRetryCount + 1) {
+	if fooBarRunTimes != (DefaultPushOrPullRetryCount + 1) {
 		t.Errorf("Unexpect run times : %d, we expect run three times", fooBarRunTimes)
 	}
 
@@ -184,6 +202,52 @@ func TestPushImage(t *testing.T) {
 	defer func() { fooBarRunTimes = 0 }()
 }
 
+func TestPullImage(t *testing.T) {
+	var testImageName string
+
+	bakRetryCount := DefaultPushOrPullRetryCount
+	bakRetryDelay := DefaultPushOrPullRetryDelay
+
+	fakeDocker := NewFakeDockerClient()
+	fakeDocker.pullImageFunc = fakePullImageFunc
+	testAuth := docker.AuthConfiguration{
+		Username:      "username_foo_bar",
+		Password:      "password_foo_bar",
+		Email:         "email_foo_bar",
+		ServerAddress: "serveraddress_foo_bar",
+	}
+
+	//make test quickly, and recover the value after testing
+	DefaultPushOrPullRetryCount = 2
+	defer func() { DefaultPushOrPullRetryCount = bakRetryCount }()
+	DefaultPushOrPullRetryDelay = 1
+	defer func() { DefaultPushOrPullRetryDelay = bakRetryDelay }()
+
+	//expect succ
+	testImageName = "repo_test_succ_foo_bar"
+	if err := pullImage(fakeDocker, testImageName, testAuth); err != nil {
+		t.Errorf("Unexpect pull image : %v, want succ", err)
+	}
+
+	//expect fail
+	testImageName = "repo_test_err_exist_foo_bar"
+	err := pullImage(fakeDocker, testImageName, testAuth)
+	if err == nil {
+		t.Errorf("Unexpect pull image : %v, want error", err)
+	}
+	//expect run 3 times
+	if fooBarRunTimes != (DefaultPushOrPullRetryCount + 1) {
+		t.Errorf("Unexpect run times : %d, we expect run three times", fooBarRunTimes)
+	}
+
+	//expect fail
+	testImageName = "repo_test_err_no_exist_foo_bar"
+	if err := pullImage(fakeDocker, testImageName, testAuth); err == nil {
+		t.Errorf("Unexpect pull image : %v, want error", err)
+	}
+	defer func() { fooBarRunTimes = 0 }()
+}
+
 func TestGetContainerNameOrID(t *testing.T) {
 	c := &docker.Container{}
 	c.ID = "ID"