Fix build controller performance issues

Calls policy completed build processing as soon as it's marked Complete
instead of doing it when the build is handled by the BuildController.
Uses a build.openshift.io/accepted annotation to bump a build so the
BuildController can see it in its queue before the next Resync.

Author: csrwng

pkg/build/api/types.go

@@ -54,6 +54,11 @@ const (
 	// BuildConfigPausedAnnotation is an annotation that marks a BuildConfig as paused.
 	// New Builds cannot be instantiated from a paused BuildConfig.
 	BuildConfigPausedAnnotation = "openshift.io/build-config.paused"
+	// BuildAcceptedAnnotation is an annotation used to update a build that can now be
+	// run based on the RunPolicy (e.g. Serial). Updating the build with this annotation
+	// forces the build to be processed by the build controller queue without waiting
+	// for a resync.
+	BuildAcceptedAnnotation = "build.openshift.io/accepted"
 )
 
 // +genclient=true

pkg/build/controller/controller.go

@@ -96,18 +96,6 @@ func (bc *BuildController) HandleBuild(build *buildapi.Build) error {
 	}
 	glog.V(4).Infof("Handling build %s/%s (%s)", build.Namespace, build.Name, build.Status.Phase)
 
-	runPolicy := policy.ForBuild(build, bc.RunPolicies)
-	if runPolicy == nil {
-		return fmt.Errorf("unable to determine build scheduler for %s/%s", build.Namespace, build.Name)
-	}
-
-	if buildutil.IsBuildComplete(build) {
-		if err := runPolicy.OnComplete(build); err != nil {
-			return err
-		}
-		return nil
-	}
-
 	// A cancelling event was triggered for the build, delete its pod and update build status.
 	if build.Status.Cancelled && build.Status.Phase != buildapi.BuildPhaseCancelled {
 		glog.V(5).Infof("Marking build %s/%s as cancelled", build.Namespace, build.Name)
@@ -126,6 +114,11 @@ func (bc *BuildController) HandleBuild(build *buildapi.Build) error {
 		return nil
 	}
 
+	runPolicy := policy.ForBuild(build, bc.RunPolicies)
+	if runPolicy == nil {
+		return fmt.Errorf("unable to determine build scheduler for %s/%s", build.Namespace, build.Name)
+	}
+
 	// The runPolicy decides whether to execute this build or not.
 	if run, err := runPolicy.IsRunnable(build); err != nil || !run {
 		return err
@@ -286,6 +279,7 @@ type BuildPodController struct {
 	BuildUpdater buildclient.BuildUpdater
 	SecretClient kcoreclient.SecretsGetter
 	PodManager   podManager
+	RunPolicies  []policy.RunPolicy
 }
 
 // HandlePod updates the state of the build based on the pod state
@@ -373,6 +367,17 @@ func (bc *BuildPodController) HandlePod(pod *kapi.Pod) error {
 			return fmt.Errorf("failed to update build %s/%s: %v", build.Namespace, build.Name, err)
 		}
 		glog.V(4).Infof("Build %s/%s status was updated %s -> %s", build.Namespace, build.Name, build.Status.Phase, nextStatus)
+
+		runPolicy := policy.ForBuild(build, bc.RunPolicies)
+		if runPolicy == nil {
+			glog.Errorf("unable to determine build scheduler for %s/%s", build.Namespace, build.Name)
+			return nil
+		}
+		if buildutil.IsBuildComplete(build) {
+			if err := runPolicy.OnComplete(build); err != nil {
+				glog.Errorf("failed to run policy on completed build: %v", err)
+			}
+		}
 	}
 	return nil
 }

pkg/build/controller/factory/factory.go

@@ -171,6 +171,7 @@ func (factory *BuildControllerFactory) CreateDeleteController() controller.Runna
 type BuildPodControllerFactory struct {
 	OSClient     osclient.Interface
 	KubeClient   kclientset.Interface
+	BuildLister  buildclient.BuildLister
 	BuildUpdater buildclient.BuildUpdater
 	// Stop may be set to allow controllers created by this factory to be terminated.
 	Stop <-chan struct{}
@@ -214,6 +215,7 @@ func (factory *BuildPodControllerFactory) Create() controller.RunnableController
 		BuildUpdater: factory.BuildUpdater,
 		SecretClient: factory.KubeClient.Core(),
 		PodManager:   client,
+		RunPolicies:  policy.GetAllRunPolicies(factory.BuildLister, factory.BuildUpdater),
 	}
 
 	return &controller.RetryController{

pkg/build/controller/policy/policy.go

@@ -5,12 +5,13 @@ import (
 	"time"
 
 	"github.com/golang/glog"
+	"github.com/pborman/uuid"
+	"k8s.io/kubernetes/pkg/api/errors"
+	"k8s.io/kubernetes/pkg/util/wait"
+
 	buildapi "github.com/openshift/origin/pkg/build/api"
 	buildclient "github.com/openshift/origin/pkg/build/client"
 	buildutil "github.com/openshift/origin/pkg/build/util"
-	"k8s.io/kubernetes/pkg/api/errors"
-	"k8s.io/kubernetes/pkg/api/unversioned"
-	"k8s.io/kubernetes/pkg/util/wait"
 )
 
 // RunPolicy is an interface that define handler for the build runPolicy field.
@@ -124,7 +125,7 @@ func GetNextConfigBuild(lister buildclient.BuildLister, namespace, buildConfigNa
 }
 
 // handleComplete represents the default OnComplete handler. This Handler will
-// check which build should be run next and update the StartTimestamp field for
+// check which build should be run next and set the accepted annotation for
 // that build. That will trigger HandleBuild() to process that build immediately
 // and as a result the build is immediately executed.
 func handleComplete(lister buildclient.BuildLister, updater buildclient.BuildUpdater, build *buildapi.Build) error {
@@ -139,9 +140,9 @@ func handleComplete(lister buildclient.BuildLister, updater buildclient.BuildUpd
 	if hasRunningBuilds || len(nextBuilds) == 0 {
 		return nil
 	}
-	now := unversioned.Now()
 	for _, build := range nextBuilds {
-		build.Status.StartTimestamp = &now
+		// TODO: replace with informer notification requeueing in the future
+		build.Annotations[buildapi.BuildAcceptedAnnotation] = uuid.NewRandom().String()
 		err := wait.Poll(500*time.Millisecond, 5*time.Second, func() (bool, error) {
 			err := updater.Update(build.Namespace, build)
 			if err != nil && errors.IsConflict(err) {

pkg/build/controller/policy/policy_test.go

@@ -112,12 +112,12 @@ func TestHandleCompleteSerial(t *testing.T) {
 		t.Errorf("unexpected error: %v", err)
 	}
 
-	if resultBuilds.Items[1].Status.StartTimestamp == nil {
-		t.Errorf("build-2 should have Status.StartTimestamp set to trigger it")
+	if _, ok := resultBuilds.Items[1].Annotations[buildapi.BuildAcceptedAnnotation]; !ok {
+		t.Errorf("build-2 should have Annotation %s set to trigger it", buildapi.BuildAcceptedAnnotation)
 	}
 
-	if resultBuilds.Items[2].Status.StartTimestamp != nil {
-		t.Errorf("build-3 should not have Status.StartTimestamp set")
+	if _, ok := resultBuilds.Items[2].Annotations[buildapi.BuildAcceptedAnnotation]; ok {
+		t.Errorf("build-3 should not have Annotation %s set", buildapi.BuildAcceptedAnnotation)
 	}
 }
 
@@ -139,11 +139,11 @@ func TestHandleCompleteParallel(t *testing.T) {
 		t.Errorf("unexpected error: %v", err)
 	}
 
-	if resultBuilds.Items[1].Status.StartTimestamp == nil {
-		t.Errorf("build-2 should have Status.StartTimestamp set to trigger it")
+	if _, ok := resultBuilds.Items[1].Annotations[buildapi.BuildAcceptedAnnotation]; !ok {
+		t.Errorf("build-2 should have Annotation %s set to trigger it", buildapi.BuildAcceptedAnnotation)
 	}
 
-	if resultBuilds.Items[2].Status.StartTimestamp == nil {
-		t.Errorf("build-3 should have Status.StartTimestamp set to trigger it")
+	if _, ok := resultBuilds.Items[2].Annotations[buildapi.BuildAcceptedAnnotation]; !ok {
+		t.Errorf("build-3 should have Annotation %s set to trigger it", buildapi.BuildAcceptedAnnotation)
 	}
 }

pkg/cmd/server/origin/run_components.go

@@ -293,6 +293,7 @@ func (c *MasterConfig) RunBuildPodController() {
 		OSClient:     osclient,
 		KubeClient:   kclient,
 		BuildUpdater: buildclient.NewOSClientBuildClient(osclient),
+		BuildLister:  buildclient.NewOSClientBuildClient(osclient),
 	}
 	controller := factory.Create()
 	controller.Run()