Merge pull request #17498 from csrwng/fix_build_controller

Automatic merge from submit-queue.

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

Current code will only retry a buildconfig if there's a running build *and* there's no pending builds waiting to be started. The conditions should be independent (*or*ed). If there's no pending builds it may be that the  cache hasn't caught up.

This fix separates the conditions and logs different messages for each. 

When an active build is deleted, we should also poke the buildconfig to start the next build.

Fixes #17353

Author: openshift-merge-robot

pkg/build/controller/build/build_controller.go

@@ -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,
@@ -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)
+		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 {
@@ -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)

test/extended/builds/run_policy.go

@@ -1,6 +1,7 @@
 package builds
 
 import (
+	"fmt"
 	"strings"
 	"time"
 
@@ -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")