cancel binary builds if they hang #12484
Conversation
bparees
changed the title
| build, err = h.r.Generator.Client.GetBuild(h.ctx, build.Name) | ||
| return false, err | ||
| } | ||
| return true, err |
There was a problem hiding this comment.
Fold this into the switch so that err == nil is default:
| // Attempt to cancel the build if it did not start running | ||
| // before we gave up. | ||
| success := false | ||
| defer func() { |
There was a problem hiding this comment.
Make this a method.
| } | ||
| build.Status.Cancelled = true | ||
| h.r.Generator.Client.UpdateBuild(h.ctx, build) | ||
| wait.Poll(500*time.Millisecond, 30*time.Second, func() (bool, error) { |
There was a problem hiding this comment.
Make these constants outside the loop but private
| @@ -178,7 +178,31 @@ func (h *binaryInstantiateHandler) handle(r io.Reader) (runtime.Object, error) { | |||
| } | |||
| remaining := h.r.Timeout - time.Now().Sub(start) | |||
|
|
|||
| // Attempt to cancel the build if it did not start running | |||
There was a problem hiding this comment.
Create an e2e test.
There was a problem hiding this comment.
any suggestions on how to create one that doesn't have to wait 5 minutes to timeout?
since the timeout isn't configurable and can't easily be made configurable from the client side.
There was a problem hiding this comment.
Hrm. Certainly ugly to test, but also ugly not to have any sort of certainly this code works.
bparees
force-pushed
the
binary_timeout
branch
from
b11b82b to
506bc8d
Compare
|
[test] |
bparees
force-pushed
the
binary_timeout
branch
4 times, most recently
from
2be53cd to
eeaea5f
Compare
|
extended builds passed: [testextended][extended:core(image_ecosystem)] |
|
@smarterclayton ready for final review, e2e test added. and yes, it waits 5 minutes. |
bparees
changed the title
|
@smarterclayton bump |
| // cancelBuild will mark a build for cancellation unless | ||
| // cancel is false in which case it is a no-op. | ||
| func (h *binaryInstantiateHandler) cancelBuild(cancel bool, build *buildapi.Build) { | ||
| if !cancel { |
There was a problem hiding this comment.
This is not necessary, don't do it.
There was a problem hiding this comment.
Just avoid calling the method.
There was a problem hiding this comment.
you're the one who wanted the defer logic wrapped into a func in the first place, now you're having me factor the func logic back out of the func....
| @@ -194,6 +194,23 @@ var _ = g.Describe("[builds][Slow] starting a build using CLI", func() { | |||
| }) | |||
| }) | |||
|
|
|||
| g.Describe("cancel a binary build that doesn't start running in 5 minutes", func() { | |||
There was a problem hiding this comment.
the whole suite is already tagged slow.
There was a problem hiding this comment.
|
Few small comments, otherwise LGTM |
bparees
force-pushed
the
binary_timeout
branch
from
eeaea5f to
be1b942
Compare
|
Evaluated for origin test up to be1b942 |
|
[testextended][extended:core(builds)] |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13200/) (Base Commit: 6bba78c) |
|
Evaluated for origin testextended up to be1b942 |
|
continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1021/) (Base Commit: b61bed3) (Extended Tests: core(builds), core(image_ecosystem)) |
|
[merge] |
|
Evaluated for origin merge up to be1b942 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13223/) (Base Commit: 871952a) (Image: devenv-rhel7_5759) |
fixes #12438