Refactor Build Controller to use Informers #14289

csrwng · 2017-05-23T01:46:13Z

Collapses the build and build pod controller into a single controller using informers

csrwng · 2017-05-23T01:48:59Z

[test]

csrwng · 2017-05-23T01:53:09Z

@bparees I'm still working on unit tests, but wanted to get this up sooner than later; ptal

bparees

my main concern is around the possibility of re-introducing races when updating the build status reason/message fields.

bparees · 2017-05-23T18:45:30Z

pkg/build/controller/build/build_controller.go

+	eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: v1core.New(params.KubeClientExternal.Core().RESTClient()).Events("")})
+
+	buildListerUpdater := buildclient.NewOSClientBuildClient(params.OpenshiftClient)
+	buildDeleter := buildclient.NewBuildDeleter(params.OpenshiftClient)


is there a good reason that NewOSClientBuildClient{} doesn't include implement Delete itself, rather than us having a separate buildDeleter{}?

Nope, we can certainly change that

bparees · 2017-05-23T18:47:48Z

pkg/build/controller/build/build_controller.go

+	}
+
+	<-stopCh
+	glog.Infof("Shutting down build pod controller")


build controller.

bparees · 2017-05-23T18:52:49Z

pkg/build/controller/build/build_controller.go

+	return nil
+}
+
+func shouldIgnore(build *buildapi.Build) bool {


bparees · 2017-05-23T18:59:41Z

pkg/build/controller/build/build_controller.go

+	return update, nil
+}
+
+func (bc *BuildController) handleNewBuild(build *buildapi.Build, willBeDropped bool, pod *kapi.Pod, podErr error) (*buildUpdate, error) {


bparees · 2017-05-23T19:00:58Z

pkg/build/controller/build/build_controller.go

+	return bc.createBuildPod(build)
+}
+
+func (bc *BuildController) createPodSpec(originalBuild *buildapi.Build, ref string) (*kapi.Pod, error) {


bparees · 2017-05-23T19:32:05Z

pkg/build/controller/build/creationstrategy.go

@@ -0,0 +1,46 @@
+package build


filename: podcreationstrategy.go

otherwise i'd expect this to create builds, given the package it's in.

bparees · 2017-05-23T19:32:46Z

pkg/build/controller/build/creationstrategy.go

+	case build.Spec.Strategy.CustomStrategy != nil:
+		pod, err = f.customBuildStrategy.CreateBuildPod(build)
+	case build.Spec.Strategy.JenkinsPipelineStrategy != nil:
+		return nil, nil


it should be an error if we get here.

bparees · 2017-05-23T19:35:15Z

pkg/cmd/server/bootstrappolicy/controller_policy.go

-			rbac.NewRule("get").Groups(imageGroup, legacyImageGroup).Resources("imagestreams").RuleOrDie(),
-			rbac.NewRule("get", "list", "create", "delete").Groups(kapiGroup).Resources("pods").RuleOrDie(),
+			rbac.NewRule("get", "list").Groups(imageGroup, legacyImageGroup).Resources("imagestreams").RuleOrDie(),
+			rbac.NewRule("get", "list").Groups(imageGroup, legacyImageGroup).Resources("secrets").RuleOrDie(),


group looks wrong for secrets?

bparees · 2017-05-23T19:37:16Z

pkg/cmd/server/origin/controller/build.go

@@ -7,8 +7,7 @@ import (

 	builddefaults "github.com/openshift/origin/pkg/build/admission/defaults"
 	buildoverrides "github.com/openshift/origin/pkg/build/admission/overrides"
-	buildclient "github.com/openshift/origin/pkg/build/client"
-	buildcontrollerfactory "github.com/openshift/origin/pkg/build/controller/factory"
+	buildcontroller "github.com/openshift/origin/pkg/build/controller/build"


is there a compelling reason not to fold this whole file into build_controller.go?

this is part of @deads2k's recent change to the build controller. I believe this is a pattern we want to following going forward?

hm. it looks weird to me, but i'll ignore it for now.

Eventually we want to push this wiring down into sub-repos, but it wasn't a primary objective and we don't have a solid interface yet. Right now, the it would create really strange interdependencies.

bparees · 2017-05-23T19:39:47Z

pkg/cmd/server/start/start_master.go

@@ -754,7 +754,6 @@ func startControllers(oc *origin.MasterConfig, kc *kubernetes.MasterConfig) erro

 	// no special order
 	if configapi.IsBuildEnabled(&oc.Options) {
-		oc.RunBuildPodController()


i'd like to understand why this block is special, instead of handling the buildconfigchangecontroller in the for loop above.

the loop above is for the new style of controllers (where they live in an array and initialized via a function in that array) (ala kube). We have not yet changed the config change controller to initialize in that way.

csrwng · 2017-05-24T01:56:47Z

@bparees please take a look at the last commit. I addressed your comments and changed the way we handle details updates from the pod to use annotations. I really don't like the potential for races on the build phase/reason/message. I'd rather have the build controller be the only one that can update those.

bparees

couple nits but i like it and thanks for the speedy turnaround!

bparees · 2017-05-24T03:16:21Z

pkg/build/controller/build/build_controller.go


 	return update, nil
 }

+// handleNewBuild will check whether policy allows running the new build and if so, creates a pod
+// for the build and returns and update to move it to the Pending phase


bparees · 2017-05-24T03:18:45Z

pkg/build/controller/build/build_controller.go

+	}
+	if _, ok := build.Annotations[buildapi.BuildPodRequestedMessageAnnotation]; ok {
+		update.setMessage(build.Annotations[buildapi.BuildPodRequestedMessageAnnotation])
+	}


cool, i like this!

bparees · 2017-05-24T03:25:42Z

pkg/build/registry/build/strategy.go

+
+	newBuild.Annotations[api.BuildPodRequestedPhaseAnnotation] = string(phase)
+	newBuild.Annotations[api.BuildPodRequestedReasonAnnotation] = string(reason)
+	newBuild.Annotations[api.BuildPodRequestedMessageAnnotation] = message


godoc needs updating w/ this behavior change.

csrwng · 2017-05-24T16:52:13Z

Updates made, new annotation constants will require @openshift/api-review

csrwng · 2017-05-25T01:08:19Z

[test]

csrwng · 2017-05-25T03:50:34Z

@bparees still don't have a clean test. Latest run has one extended test failing...
Optimized image builds should succeed as an admin [Conformance]
there's also an integration build that failed, but succeeded in previous runs ... more like a flake
TestProjectLifecycle

csrwng · 2017-05-25T17:28:45Z

[testextended][extended:core(builds)]

csrwng · 2017-05-26T12:33:19Z

Flakes #14368 #13980

csrwng · 2017-05-26T12:41:54Z

Just realized I didn't tag the entire team... apologies. @openshift/devex ptal

csrwng · 2017-06-05T13:29:58Z

@openshift/api-review bump

bparees · 2017-06-11T03:44:46Z

add the code comment and i think it's ready for merge. feel free to put the tag on yourself after you add the comment.

csrwng · 2017-06-11T21:10:11Z

Removed special case for pending transition
[merge][severity:blocker]

mfojtik · 2017-06-12T12:52:41Z

pkg/build/controller/strategy/util.go

+	t := true
+	pod.OwnerReferences = []metav1.OwnerReference{
+		{
+			APIVersion: "v1",


must be build.openshift.io/v1
`

also consider something like:

https://github.com/openshift/origin/pull/14582/files#diff-d09310d3307d040cedbddcd5fa308067R30

bparees · 2017-06-12T12:53:37Z

pkg/build/controller/strategy/util.go

+	t := true
+	pod.OwnerReferences = []metav1.OwnerReference{
+		{
+			APIVersion: "v1",


@mfojtik noted that this needs to be buildapiv1.SchemeGroupVersion

csrwng · 2017-06-12T14:10:38Z

Thx @mfojtik and @bparees. Code updated.

csrwng · 2017-06-12T15:07:47Z

rebased and fixed compile error with ImageStream informer

csrwng · 2017-06-12T18:23:50Z

Fixed unit test to use generated informer for ImageStreams

csrwng · 2017-06-13T02:19:27Z

Flake #13984 on extended test

csrwng · 2017-06-13T02:20:30Z

[testextended][extended:core(builds)]

openshift-bot · 2017-06-13T02:29:38Z

Evaluated for origin testextended up to 3091d6a

openshift-bot · 2017-06-13T05:13:41Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/613/) (Base Commit: 0ab5ccf) (Extended Tests: core(builds))

smarterclayton · 2017-06-15T05:08:13Z

[test]

openshift-bot · 2017-06-15T05:13:47Z

Evaluated for origin test up to 3091d6a

smarterclayton · 2017-06-15T05:42:14Z

[merge]

openshift-bot · 2017-06-15T05:45:42Z

Evaluated for origin merge up to 3091d6a

openshift-bot · 2017-06-15T05:45:49Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1003/) (Base Commit: eb50c84) (Extended Tests: blocker) (Image: devenv-rhel7_6361)

openshift-bot · 2017-06-15T07:14:16Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2254/) (Base Commit: 811a267)

csrwng force-pushed the build_controller branch from d87f7af to a73e660 Compare May 23, 2017 15:13

bparees requested changes May 23, 2017

View reviewed changes

csrwng force-pushed the build_controller branch from b4ada8a to 4965647 Compare May 23, 2017 22:43

bparees requested changes May 24, 2017

View reviewed changes

csrwng force-pushed the build_controller branch 4 times, most recently from a65f31b to 46ed2ab Compare May 24, 2017 16:45

csrwng added the needs-api-review label May 24, 2017

csrwng force-pushed the build_controller branch 2 times, most recently from d03414f to 8b2a09b Compare May 25, 2017 00:49

csrwng changed the title ~~[WIP] Refactor Build Controller to use Informers~~ Refactor Build Controller to use Informers May 25, 2017

csrwng force-pushed the build_controller branch from 8b2a09b to 574275a Compare May 25, 2017 01:21

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2017

csrwng force-pushed the build_controller branch from 574275a to 74ba613 Compare May 25, 2017 13:20

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2017

csrwng force-pushed the build_controller branch from 74ba613 to 9e3b97c Compare May 25, 2017 15:45

csrwng force-pushed the build_controller branch from 9e3b97c to 33ed7cd Compare May 26, 2017 00:37

csrwng mentioned this pull request May 26, 2017

Support valueFrom syntax for build env vars #14143

Merged

coreydaley mentioned this pull request May 27, 2017

Add tests for BuildPhaseError #14390

Closed

csrwng force-pushed the build_controller branch from 740c44e to c3e41b4 Compare June 11, 2017 21:09

csrwng force-pushed the build_controller branch from c3e41b4 to 4848a66 Compare June 12, 2017 03:19

mfojtik reviewed Jun 12, 2017

View reviewed changes

bparees reviewed Jun 12, 2017

View reviewed changes

csrwng force-pushed the build_controller branch from 4848a66 to 7f43535 Compare June 12, 2017 14:10

csrwng force-pushed the build_controller branch 2 times, most recently from 6036be8 to 71846a9 Compare June 12, 2017 15:07

csrwng mentioned this pull request Jun 12, 2017

Refactor BuildConfig controller to use Informers #14596

Merged

csrwng force-pushed the build_controller branch from 71846a9 to 3697263 Compare June 12, 2017 18:23

csrwng force-pushed the build_controller branch from 3697263 to 3fab00c Compare June 12, 2017 19:07

Refactor Build Controller to use Informers

3091d6a

csrwng force-pushed the build_controller branch from 3fab00c to 3091d6a Compare June 12, 2017 22:11

bparees approved these changes Jun 13, 2017

View reviewed changes

openshift-bot merged commit 7ddec43 into openshift:master Jun 15, 2017

mfojtik mentioned this pull request Jun 15, 2017

Refactor controller initialization (round #3) #14317

Merged

Refactor Build Controller to use Informers #14289

Refactor Build Controller to use Informers #14289

Conversation

csrwng commented May 23, 2017

csrwng commented May 23, 2017

csrwng commented May 23, 2017

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented May 24, 2017

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented May 24, 2017

csrwng commented May 25, 2017

csrwng commented May 25, 2017

csrwng commented May 25, 2017

csrwng commented May 26, 2017

csrwng commented May 26, 2017

csrwng commented Jun 5, 2017

bparees commented Jun 11, 2017

csrwng commented Jun 11, 2017 • edited by bparees Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented Jun 12, 2017

csrwng commented Jun 12, 2017

csrwng commented Jun 12, 2017

csrwng commented Jun 13, 2017

csrwng commented Jun 13, 2017

openshift-bot commented Jun 13, 2017

openshift-bot commented Jun 13, 2017

smarterclayton commented Jun 15, 2017

openshift-bot commented Jun 15, 2017

smarterclayton commented Jun 15, 2017

openshift-bot commented Jun 15, 2017

openshift-bot commented Jun 15, 2017 • edited Loading

openshift-bot commented Jun 15, 2017

csrwng commented Jun 11, 2017 •

edited by bparees

Loading

openshift-bot commented Jun 15, 2017 •

edited

Loading