Skip to content

Commit 506bc8d

Browse files
bpareesbparees
committed
cancel binary builds if they hang
1 parent 4bae8ee commit 506bc8d

File tree

13 files changed

+139
-27
lines changed

13 files changed

+139
-27
lines changed

‎pkg/build/generator/generator.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ type GeneratorClient interface {
5757
UpdateBuildConfig(ctx kapi.Context, buildConfig *buildapi.BuildConfig) error
5858
GetBuild(ctx kapi.Context, name string) (*buildapi.Build, error)
5959
CreateBuild(ctx kapi.Context, build *buildapi.Build) error
60+
UpdateBuild(ctx kapi.Context, build *buildapi.Build) error
6061
GetImageStream(ctx kapi.Context, name string) (*imageapi.ImageStream, error)
6162
GetImageStreamImage(ctx kapi.Context, name string) (*imageapi.ImageStreamImage, error)
6263
GetImageStreamTag(ctx kapi.Context, name string) (*imageapi.ImageStreamTag, error)
@@ -68,6 +69,7 @@ type Client struct {
6869
UpdateBuildConfigFunc func(ctx kapi.Context, buildConfig *buildapi.BuildConfig) error
6970
GetBuildFunc func(ctx kapi.Context, name string) (*buildapi.Build, error)
7071
CreateBuildFunc func(ctx kapi.Context, build *buildapi.Build) error
72+
UpdateBuildFunc func(ctx kapi.Context, build *buildapi.Build) error
7173
GetImageStreamFunc func(ctx kapi.Context, name string) (*imageapi.ImageStream, error)
7274
GetImageStreamImageFunc func(ctx kapi.Context, name string) (*imageapi.ImageStreamImage, error)
7375
GetImageStreamTagFunc func(ctx kapi.Context, name string) (*imageapi.ImageStreamTag, error)
@@ -93,6 +95,11 @@ func (c Client) CreateBuild(ctx kapi.Context, build *buildapi.Build) error {
9395
return c.CreateBuildFunc(ctx, build)
9496
}
9597

98+
// UpdateBuild updates a build
99+
func (c Client) UpdateBuild(ctx kapi.Context, build *buildapi.Build) error {
100+
return c.UpdateBuildFunc(ctx, build)
101+
}
102+
96103
// GetImageStream retrieves a named image stream
97104
func (c Client) GetImageStream(ctx kapi.Context, name string) (*imageapi.ImageStream, error) {
98105
return c.GetImageStreamFunc(ctx, name)

‎pkg/build/registry/buildconfiginstantiate/rest.go

Lines changed: 51 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,11 @@ import (
2727
buildutil "github.com/openshift/origin/pkg/build/util"
2828
)
2929

30+
var (
31+
cancelPollInterval = 500 * time.Millisecond
32+
cancelPollDuration = 30 * time.Second
33+
)
34+
3035
// NewStorage creates a new storage object for build generation
3136
func NewStorage(generator *generator.BuildGenerator) *InstantiateREST {
3237
return &InstantiateREST{generator: generator}
@@ -178,24 +183,31 @@ func (h *binaryInstantiateHandler) handle(r io.Reader) (runtime.Object, error) {
178183
}
179184
remaining := h.r.Timeout - time.Now().Sub(start)
180185

186+
// Attempt to cancel the build if it did not start running
187+
// before we gave up.
188+
cancel := true
189+
defer h.cancelBuild(cancel, build)
190+
181191
latest, ok, err := registry.WaitForRunningBuild(h.r.Watcher, h.ctx, build, remaining)
182-
if err != nil {
183-
switch {
184-
case latest.Status.Phase == buildapi.BuildPhaseError:
185-
return nil, errors.NewBadRequest(fmt.Sprintf("build %s encountered an error: %s", build.Name, buildutil.NoBuildLogsMessage))
186-
case latest.Status.Phase == buildapi.BuildPhaseCancelled:
187-
return nil, errors.NewBadRequest(fmt.Sprintf("build %s was cancelled: %s", build.Name, buildutil.NoBuildLogsMessage))
188-
case err == registry.ErrBuildDeleted:
189-
return nil, errors.NewBadRequest(fmt.Sprintf("build %s was deleted before it started: %s", build.Name, buildutil.NoBuildLogsMessage))
190-
default:
191-
return nil, errors.NewBadRequest(fmt.Sprintf("unable to wait for build %s to run: %v", build.Name, err))
192-
}
193-
}
194-
if !ok {
195-
return nil, errors.NewTimeoutError(fmt.Sprintf("timed out waiting for build %s to start after %s", build.Name, h.r.Timeout), 0)
196-
}
197-
if latest.Status.Phase != buildapi.BuildPhaseRunning {
192+
193+
switch {
194+
case latest.Status.Phase == buildapi.BuildPhaseError:
195+
return nil, errors.NewBadRequest(fmt.Sprintf("build %s encountered an error: %s", build.Name, buildutil.NoBuildLogsMessage))
196+
case latest.Status.Phase == buildapi.BuildPhaseFailed:
197+
// don't cancel the build if it reached a terminal state on its own
198+
cancel = false
199+
return nil, errors.NewBadRequest(fmt.Sprintf("build %s failed: %s: %s", build.Name, build.Status.Reason, build.Status.Message))
200+
case latest.Status.Phase == buildapi.BuildPhaseCancelled:
201+
cancel = false
202+
return nil, errors.NewBadRequest(fmt.Sprintf("build %s was cancelled: %s", build.Name, buildutil.NoBuildLogsMessage))
203+
case latest.Status.Phase != buildapi.BuildPhaseRunning:
198204
return nil, errors.NewBadRequest(fmt.Sprintf("cannot upload file to build %s with status %s", build.Name, latest.Status.Phase))
205+
case err == registry.ErrBuildDeleted:
206+
return nil, errors.NewBadRequest(fmt.Sprintf("build %s was deleted before it started: %s", build.Name, buildutil.NoBuildLogsMessage))
207+
case err != nil:
208+
return nil, errors.NewBadRequest(fmt.Sprintf("unable to wait for build %s to run: %v", build.Name, err))
209+
case !ok:
210+
return nil, errors.NewTimeoutError(fmt.Sprintf("timed out waiting for build %s to start after %s", build.Name, h.r.Timeout), 0)
199211
}
200212

201213
// The container should be the default build container, so setting it to blank
@@ -226,9 +238,32 @@ func (h *binaryInstantiateHandler) handle(r io.Reader) (runtime.Object, error) {
226238
if err := exec.Stream(streamOptions); err != nil {
227239
return nil, errors.NewInternalError(err)
228240
}
241+
cancel = false
229242
return latest, nil
230243
}
231244

245+
// cancelBuild will mark a build for cancellation unless
246+
// cancel is false in which case it is a no-op.
247+
func (h *binaryInstantiateHandler) cancelBuild(cancel bool, build *buildapi.Build) {
248+
if !cancel {
249+
return
250+
}
251+
252+
build.Status.Cancelled = true
253+
h.r.Generator.Client.UpdateBuild(h.ctx, build)
254+
wait.Poll(cancelPollInterval, cancelPollDuration, func() (bool, error) {
255+
build.Status.Cancelled = true
256+
err := h.r.Generator.Client.UpdateBuild(h.ctx, build)
257+
switch {
258+
case err != nil && errors.IsConflict(err):
259+
build, err = h.r.Generator.Client.GetBuild(h.ctx, build.Name)
260+
return false, err
261+
default:
262+
return true, err
263+
}
264+
})
265+
}
266+
232267
type podGetter struct {
233268
podsNamespacer kcoreclient.PodsGetter
234269
}

‎pkg/cmd/server/origin/master.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -619,6 +619,7 @@ func (c *MasterConfig) GetRestStorage() map[string]rest.Storage {
619619
UpdateBuildConfigFunc: buildConfigRegistry.UpdateBuildConfig,
620620
GetBuildFunc: buildRegistry.GetBuild,
621621
CreateBuildFunc: buildRegistry.CreateBuild,
622+
UpdateBuildFunc: buildRegistry.UpdateBuild,
622623
GetImageStreamFunc: imageStreamRegistry.GetImageStream,
623624
GetImageStreamImageFunc: imageStreamImageRegistry.GetImageStreamImage,
624625
GetImageStreamTagFunc: imageStreamTagRegistry.GetImageStreamTag,

‎test/extended/builds/dockerfile.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ USER 1001
4646

4747
buildName := "jenkins-1"
4848
g.By("expecting the Dockerfile build is in Complete phase")
49-
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), buildName, exutil.CheckBuildSuccessFn, exutil.CheckBuildFailedFn)
49+
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), buildName, nil, nil, nil)
5050
//debug for failures on jenkins
5151
if err != nil {
5252
exutil.DumpBuildLogs("jenkins", oc)
@@ -75,7 +75,7 @@ USER 1001
7575

7676
buildName := "centos-1"
7777
g.By("expecting the Dockerfile build is in Complete phase")
78-
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), buildName, exutil.CheckBuildSuccessFn, exutil.CheckBuildFailedFn)
78+
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), buildName, nil, nil, nil)
7979
//debug for failures on jenkins
8080
if err != nil {
8181
exutil.DumpBuildLogs("centos", oc)

‎test/extended/builds/gitauth.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ var _ = g.Describe("[builds][Slow] can use private repositories as build input",
8686
o.Expect(err).NotTo(o.HaveOccurred())
8787
8888
g.By(fmt.Sprintf("expecting build %s to complete successfully", buildName))
89-
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), buildName, exutil.CheckBuildSuccessFn, exutil.CheckBuildFailedFn)
89+
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), buildName, nil, nil, nil)
9090
if err != nil {
9191
exutil.DumpBuildLogs(buildConfigName, oc)
9292
}

‎test/extended/builds/start.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ var _ = g.Describe("[builds][Slow] starting a build using CLI", func() {
103103
o.Expect(br.StartBuildStdErr).To(o.ContainSubstring("Uploading file"))
104104
o.Expect(br.StartBuildStdErr).To(o.ContainSubstring("as binary input for the build ..."))
105105
o.Expect(buildLog).To(o.ContainSubstring("Your bundle is complete"))
106-
107106
})
108107

109108
g.It("should accept --from-dir as input", func() {
@@ -192,6 +191,14 @@ var _ = g.Describe("[builds][Slow] starting a build using CLI", func() {
192191
o.Expect(br.StartBuildStdErr).To(o.ContainSubstring(fmt.Sprintf("Uploading archive from %q as binary input for the build", exampleArchiveURL)))
193192
o.Expect(buildLog).To(o.ContainSubstring("Your bundle is complete"))
194193
})
194+
195+
g.It("should cancel a build that doesn't start running in 5 minutes", func() {
196+
g.By("starting a build with a nodeselector that can't be matched")
197+
br, err := exutil.StartBuildAndWait(oc, "sample-build-binary-invalidnodeselector", fmt.Sprintf("--from-file=%s", exampleGemfile))
198+
o.Expect(err).NotTo(o.HaveOccurred())
199+
o.Expect(br.BuildCancelled).To(o.BeTrue(), "build should be canceled")
200+
})
201+
195202
})
196203

197204
g.Describe("cancelling build started by oc start-build --wait", func() {

‎test/extended/image_ecosystem/s2i_perl.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ var _ = g.Describe("[image_ecosystem][perl][Slow] hot deploy for openshift perl
3333
o.Expect(err).NotTo(o.HaveOccurred())
3434

3535
g.By("waiting for build to finish")
36-
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), "dancer-mysql-example-1", exutil.CheckBuildSuccessFn, exutil.CheckBuildFailedFn)
36+
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), "dancer-mysql-example-1", nil, nil, nil)
3737
if err != nil {
3838
exutil.DumpBuildLogs("dancer-mysql-example", oc)
3939
}

‎test/extended/image_ecosystem/s2i_php.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ var _ = g.Describe("[image_ecosystem][php][Slow] hot deploy for openshift php im
3131
o.Expect(err).NotTo(o.HaveOccurred())
3232

3333
g.By("waiting for build to finish")
34-
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), dcName, exutil.CheckBuildSuccessFn, exutil.CheckBuildFailedFn)
34+
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), dcName, nil, nil, nil)
3535
if err != nil {
3636
exutil.DumpBuildLogs("cakephp-mysql-example", oc)
3737
}

‎test/extended/image_ecosystem/s2i_python.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ var _ = g.Describe("[image_ecosystem][python][Slow] hot deploy for openshift pyt
3434
o.Expect(err).NotTo(o.HaveOccurred())
3535

3636
g.By("waiting for build to finish")
37-
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), "django-ex-1", exutil.CheckBuildSuccessFn, exutil.CheckBuildFailedFn)
37+
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), "django-ex-1", nil, nil, nil)
3838
if err != nil {
3939
exutil.DumpBuildLogs("django-ex", oc)
4040
}

‎test/extended/image_ecosystem/s2i_ruby.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ var _ = g.Describe("[image_ecosystem][ruby][Slow] hot deploy for openshift ruby
3232
o.Expect(err).NotTo(o.HaveOccurred())
3333

3434
g.By("waiting for build to finish")
35-
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), dcName, exutil.CheckBuildSuccessFn, exutil.CheckBuildFailedFn)
35+
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), dcName, nil, nil, nil)
3636
if err != nil {
3737
exutil.DumpBuildLogs("rails-postgresql-example", oc)
3838
}

‎test/extended/image_ecosystem/sample_repos.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func NewSampleRepoTest(c SampleRepoConfig) func() {
5050
buildName := c.buildConfigName + "-1"
5151

5252
g.By("expecting the build is in the Complete phase")
53-
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), buildName, exutil.CheckBuildSuccessFn, exutil.CheckBuildFailedFn)
53+
err = exutil.WaitForABuild(oc.Client().Builds(oc.Namespace()), buildName, nil, nil, nil)
5454
if err != nil {
5555
exutil.DumpBuildLogs(c.buildConfigName, oc)
5656
}

‎test/extended/testdata/test-build.json

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,45 @@
171171
"status": {
172172
"lastVersion": 0
173173
}
174+
},
175+
{
176+
"kind": "BuildConfig",
177+
"apiVersion": "v1",
178+
"metadata": {
179+
"name": "sample-build-binary-invalidnodeselector",
180+
"creationTimestamp": null
181+
},
182+
"spec": {
183+
"triggers": [
184+
{
185+
"type": "imageChange",
186+
"imageChange": {}
187+
}
188+
],
189+
"source": {
190+
"type": "Binary",
191+
"binary": {}
192+
},
193+
"strategy": {
194+
"type": "Docker",
195+
"dockerStrategy": {
196+
"env": [
197+
{ "name": "FOO", "value": "test" },
198+
{ "name": "BAR", "value": "test" },
199+
{ "name": "BUILD_LOGLEVEL", "value": "5" }
200+
],
201+
"from": {
202+
"kind": "DockerImage",
203+
"name": "centos/ruby-22-centos7"
204+
}
205+
}
206+
},
207+
"resources": {},
208+
"nodeSelectors": {"nodelabelkey":"nodelabelvalue"}
209+
},
210+
"status": {
211+
"lastVersion": 0
212+
}
174213
}
175214
]
176215
}

‎test/extended/util/framework.go

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,8 @@ type BuildResult struct {
302302
BuildSuccess bool
303303
// BuildFailure is true if the build was finished with an error.
304304
BuildFailure bool
305+
// BuildCancelled is true if the build was canceled.
306+
BuildCancelled bool
305307
// BuildTimeout is true if there was a timeout waiting for the build to finish.
306308
BuildTimeout bool
307309
// The openshift client which created this build.
@@ -400,6 +402,7 @@ func StartBuildAndWait(oc *CLI, args ...string) (result *BuildResult, err error)
400402
BuildAttempt: false,
401403
BuildSuccess: false,
402404
BuildFailure: false,
405+
BuildCancelled: false,
403406
BuildTimeout: false,
404407
oc: oc,
405408
}
@@ -428,6 +431,11 @@ func StartBuildAndWait(oc *CLI, args ...string) (result *BuildResult, err error)
428431
result.BuildFailure = CheckBuildFailedFn(b)
429432
return result.BuildFailure
430433
},
434+
func(b *buildapi.Build) bool {
435+
result.Build = b
436+
result.BuildCancelled = CheckBuildCancelledFn(b)
437+
return result.BuildCancelled
438+
},
431439
)
432440

433441
if result.Build == nil {
@@ -443,7 +451,17 @@ func StartBuildAndWait(oc *CLI, args ...string) (result *BuildResult, err error)
443451
}
444452

445453
// WaitForABuild waits for a Build object to match either isOK or isFailed conditions.
446-
func WaitForABuild(c client.BuildInterface, name string, isOK, isFailed func(*buildapi.Build) bool) error {
454+
func WaitForABuild(c client.BuildInterface, name string, isOK, isFailed, isCanceled func(*buildapi.Build) bool) error {
455+
if isOK == nil {
456+
isOK = CheckBuildSuccessFn
457+
}
458+
if isFailed == nil {
459+
isFailed = CheckBuildFailedFn
460+
}
461+
if isCanceled == nil {
462+
isCanceled = CheckBuildCancelledFn
463+
}
464+
447465
// wait 2 minutes for build to exist
448466
err := wait.Poll(1*time.Second, 2*time.Minute, func() (bool, error) {
449467
if _, err := c.Get(name); err != nil {
@@ -464,7 +482,7 @@ func WaitForABuild(c client.BuildInterface, name string, isOK, isFailed func(*bu
464482
return false, err
465483
}
466484
for i := range list.Items {
467-
if name == list.Items[i].Name && isOK(&list.Items[i]) {
485+
if name == list.Items[i].Name && (isOK(&list.Items[i]) || isCanceled(&list.Items[i])) {
468486
return true, nil
469487
}
470488
if name != list.Items[i].Name || isFailed(&list.Items[i]) {
@@ -489,6 +507,11 @@ var CheckBuildFailedFn = func(b *buildapi.Build) bool {
489507
return b.Status.Phase == buildapi.BuildPhaseFailed || b.Status.Phase == buildapi.BuildPhaseError
490508
}
491509

510+
// CheckBuildCancelledFn return true if the build was canceled
511+
var CheckBuildCancelledFn = func(b *buildapi.Build) bool {
512+
return b.Status.Phase == buildapi.BuildPhaseCancelled
513+
}
514+
492515
// WaitForBuilderAccount waits until the builder service account gets fully
493516
// provisioned
494517
func WaitForBuilderAccount(c kcoreclient.ServiceAccountInterface) error {

0 commit comments

Comments
 (0)