-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Allow retry action of pulling image in build process #13380
Allow retry action of pulling image in build process #13380
Conversation
pkg/build/builder/dockerutil.go
Outdated
if err := RetryImageAction(client, opts, authConfig); err != nil { | ||
return "", err | ||
} else { | ||
return digestWriter.Digest, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no else block needed, just "return digestWriter.Digest,nil"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
[test] |
[testextended][extended:core(builds)] |
test falilure seems a flake
|
That's not the failure, the failure was in a push image test, which given
the area of your changes is probably not a flake.
Ben Parees | OpenShift
…On Mar 15, 2017 9:56 PM, "Guangxu Li" ***@***.***> wrote:
@bparees <https://github.com/bparees>
test falilure seems a flake
[ERROR] PID 12365: hack/lib/build/environment.sh:137: `return "${exitcode}"` exited with status 2.
13:02:27 [INFO] Stack Trace:
13:02:27 [INFO] 1: hack/lib/build/environment.sh:137: `return "${exitcode}"`
13:02:27 [INFO] 2: hack/lib/build/environment.sh:217: os::build::environment::withsource
13:02:27 [INFO] 3: hack/env:36: os::build::environment::run
13:02:27 [INFO] Exiting with code 2.
13:02:27 ++ export status=FAILURE
13:02:27 ++ status=FAILURE
13:02:27 + set +o xtrace
13:02:27
########## FINISHED STAGE: FAILURE: RUN CHECK AND VERIFY TASKS ##########
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13380 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEvl3lEfFFFghnckMjTL78S93q4Io-91ks5rmJazgaJpZM4Mb-wS>
.
|
be2e5e6
to
bce7095
Compare
@bparees sorry, in my local test I did not check the test error carefully. |
@@ -141,8 +141,8 @@ func TestTagImage(t *testing.T) { | |||
func TestPushImage(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a TestPullImage equivalent to this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/build/builder/dockerutil.go
Outdated
time.Sleep(DefaultPushOrPullRetryDelay) | ||
} | ||
|
||
return fmt.Errorf("After retry %d times, %s image still failed", DefaultPushOrPullRetryDelay, actionName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retry->retrying
bce7095
to
c4afcce
Compare
@bparees Test case addressed, ptal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last nit.
pkg/build/builder/dockerutil_test.go
Outdated
fakeDocker := NewFakeDockerClient() | ||
fakeDocker.pullImageFunc = fakePullImageFunc | ||
testAuth := docker.AuthConfiguration{ | ||
Username: "usernname_foo_bar", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
username_foo_bar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
[testextended][extended:core(builds)] |
[test] |
Evaluated for origin testextended up to c4afcce |
delete else block return error after retry maxtimes add test case for pull image fix username
c4afcce
to
ca07fa9
Compare
@bparees conformance test fail, the failure cause seems not related to what this PR does. |
agree, looks like a flake. |
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/48/) (Base Commit: 75c6750) (Extended Tests: core(builds)) |
[merge] |
Evaluated for origin merge up to ca07fa9 |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to ca07fa9 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/288/) (Base Commit: fae18e0) |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/131/) (Base Commit: 65a0cbf) (Image: devenv-rhel7_6083) |
Origin Action Required: Please contact #openshift-dev to have this pull request manually reviewed and tested |
command :
[cloud@centos ~]$ oc new-build https://github.com/openshift/ruby-hello-world --source-image=openshift/jenkins-1-centos7 --source-image-path=/var/lib/jenkins:tmp --allow-missing-images=false
result:
--> Found Docker image bce81d0 (2 weeks old) from Docker Hub for "centos/ruby-22-centos7"
Log:
Since the error connection reset by peer had been handled in build pushing process,
I think we should add retry action of pulling image like what pushing image does. Test result seems acceptable which always successful after retrying pull image 2~3 time in my workaround.
/cc @bparees