Skip to content
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

build controller: fix start of serial builds on cancellation/deletion #17498

Merged
merged 1 commit into from
Dec 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion pkg/build/controller/build/build_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ func NewBuildController(params *BuildControllerParams) *BuildController {
c.buildInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
AddFunc: c.buildAdded,
UpdateFunc: c.buildUpdated,
DeleteFunc: c.buildDeleted,
})
params.ImageStreamInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
AddFunc: c.imageStreamAdded,
Expand Down Expand Up @@ -1067,10 +1068,14 @@ func (bc *BuildController) handleBuildConfig(bcNamespace string, bcName string)
return err
}
glog.V(5).Infof("Build config %s/%s: has %d next builds, is running builds: %v", bcNamespace, bcName, len(nextBuilds), hasRunningBuilds)
if len(nextBuilds) == 0 && hasRunningBuilds {
if hasRunningBuilds {
glog.V(4).Infof("Build config %s/%s has running builds, will retry", bcNamespace, bcName)
return fmt.Errorf("build config %s/%s has running builds and cannot run more builds", bcNamespace, bcName)
}
if len(nextBuilds) == 0 {
glog.V(4).Infof("Build config %s/%s has no builds to run next, will retry", bcNamespace, bcName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will retry? doesn't this mean we could potentially end up spinning forever on a buildconfig that has no builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We give up after 15 retries

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok.

return fmt.Errorf("build config %s/%s has no builds to run next", bcNamespace, bcName)
}

// Enqueue any builds to build next
for _, build := range nextBuilds {
Expand Down Expand Up @@ -1174,6 +1179,29 @@ func (bc *BuildController) buildUpdated(old, cur interface{}) {
bc.enqueueBuild(build)
}

// buildDeleted is called by the build informer event handler whenever a build
// is deleted
func (bc *BuildController) buildDeleted(obj interface{}) {
build, ok := obj.(*buildapi.Build)
if !ok {
tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
if !ok {
utilruntime.HandleError(fmt.Errorf("couldn't get object from tombstone: %+v", obj))
return
}
build, ok = tombstone.Obj.(*buildapi.Build)
if !ok {
utilruntime.HandleError(fmt.Errorf("tombstone contained object that is not a pod: %+v", obj))
return
}
}
// If the build was not in a complete state, poke the buildconfig to run the next build
if !buildutil.IsBuildComplete(build) {
bcName := buildutil.ConfigNameForBuild(build)
bc.enqueueBuildConfig(build.Namespace, bcName)
}
}

// enqueueBuild adds the given build to the buildQueue.
func (bc *BuildController) enqueueBuild(build *buildapi.Build) {
key := resourceName(build.Namespace, build.Name)
Expand Down
53 changes: 53 additions & 0 deletions test/extended/builds/run_policy.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package builds

import (
"fmt"
"strings"
"time"

Expand Down Expand Up @@ -310,6 +311,58 @@ var _ = g.Describe("[Feature:Builds][Slow] using build configuration runPolicy",
})
})

g.Describe("build configuration with Serial build run policy handling deletion", func() {
g.It("starts the next build immediately after running one is deleted", func() {
g.By("starting multiple builds")

bcName := "sample-serial-build"
for i := 0; i < 3; i++ {
_, _, err := exutil.StartBuild(oc, bcName)
o.Expect(err).NotTo(o.HaveOccurred())
}

buildWatch, err := oc.BuildClient().Build().Builds(oc.Namespace()).Watch(metav1.ListOptions{
LabelSelector: buildutil.BuildConfigSelector(bcName).String(),
})
defer buildWatch.Stop()
o.Expect(err).NotTo(o.HaveOccurred())

var deleteTime, deleteTime2 time.Time
done := false
var timedOut error
for !done {
select {
case event := <-buildWatch.ResultChan():
build := event.Object.(*buildapi.Build)
if build.Status.Phase == buildapi.BuildPhasePending {
if build.Name == "sample-serial-build-1" {
err := oc.Run("delete").Args("build", "sample-serial-build-1", "--ignore-not-found").Execute()
o.Expect(err).ToNot(o.HaveOccurred())
deleteTime = time.Now()
}
if build.Name == "sample-serial-build-2" {
duration := time.Now().Sub(deleteTime)
o.Expect(duration).To(o.BeNumerically("<", 20*time.Second), "next build should have started less than 20s after deleted build")
err := oc.Run("delete").Args("build", "sample-serial-build-2", "--ignore-not-found").Execute()
o.Expect(err).ToNot(o.HaveOccurred())
deleteTime2 = time.Now()
}
if build.Name == "sample-serial-build-3" {
duration := time.Now().Sub(deleteTime2)
o.Expect(duration).To(o.BeNumerically("<", 20*time.Second), "next build should have started less than 20s after deleted build")
done = true
}
}
case <-time.After(2 * time.Minute):
// Give up waiting and finish after 2 minutes
timedOut = fmt.Errorf("timed out waiting for pending build")
done = true
}
}
o.Expect(timedOut).NotTo(o.HaveOccurred())
})
})

g.Describe("build configuration with SerialLatestOnly build run policy", func() {
g.It("runs the builds in serial order but cancel previous builds", func() {
g.By("starting multiple builds")
Expand Down