Refactor controller initialization (round #3) #14317

mfojtik · 2017-05-24T10:41:28Z

Follow up for #14293 with more controllers.

This time:

image trigger controller
service serving cert controller
image import controller

mfojtik · 2017-05-24T10:42:16Z

@deads2k #14293 is going to have green tests, lets continue refactoring here. i'm going to move image controllers. @soltysh FYI you don't have to do this.

mfojtik · 2017-05-24T11:08:00Z

[test]

mfojtik · 2017-06-05T18:18:53Z

[test]

mfojtik · 2017-06-05T18:19:16Z

@deads2k ready for the round 3 ?

mfojtik · 2017-06-05T18:24:25Z

@soltysh for the image controllers

mfojtik · 2017-06-05T18:25:18Z

@smarterclayton for the image trigger controller

mfojtik · 2017-06-05T18:28:37Z

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

+	return true, nil
+}
+
+// TODO: remove when generated informers exist


@deads2k @smarterclayton do they exists now? i saw some image stream generated informers already ;)

They exists, but are not wired, yet (or not everywhere) iirc. This should be removed when we entirely switch to the generated informers. The question is do we want to make it happen for 3.6?

no, that is 3.7

mfojtik · 2017-06-05T18:32:38Z

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

+			Resync: time.Duration(c.ScheduledImageImportMinimumIntervalSeconds) * time.Second,
+
+			Enabled:                  !c.DisableScheduledImport,
+			DefaultBucketSize:        4, // TODO: Make this configurable?


@soltysh do we have issue for this? do we care?

IIRC, you've added that TODO, I don't think we care. Drop the todo.

soltysh

A few nits, but LGTM

soltysh · 2017-06-06T08:47:13Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

-	InfraDeploymentConfigControllerServiceAccountName          = "deploymentconfig-controller"
-	InfraDeploymentTriggerControllerServiceAccountName         = "deployment-trigger-controller"
-	InfraDeployerControllerServiceAccountName                  = "deployer-controller"
+	// The controllers below were convered to new controller initialization and use RBAC


Typo: converted

soltysh · 2017-06-06T08:50:43Z

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

+	return true, nil
+}
+
+// TODO: remove when generated informers exist


They exists, but are not wired, yet (or not everywhere) iirc. This should be removed when we entirely switch to the generated informers. The question is do we want to make it happen for 3.6?

soltysh · 2017-06-06T08:56:43Z

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

+			Resync: time.Duration(c.ScheduledImageImportMinimumIntervalSeconds) * time.Second,
+
+			Enabled:                  !c.DisableScheduledImport,
+			DefaultBucketSize:        4, // TODO: Make this configurable?


IIRC, you've added that TODO, I don't think we care. Drop the todo.

soltysh · 2017-06-06T08:57:50Z

test/integration/buildcontroller_test.go

@@ -165,7 +165,10 @@ func setupBuildControllerTest(counts controllerCount, t *testing.T) (*client.Cli
 		}
 	}
 	for i := 0; i < counts.ImageChangeControllers; i++ {
-		openshiftConfig.RunImageTriggerController()
+		_, err := openshiftControllerInitializers["imagetrigger"](openshiftControllerContext)


I'm guessing this should be image-trigger since in the 3rd commit you're changing that.

yes, good catch... altough i'm not sure how this will not panic ;-)

deads2k · 2017-06-06T18:13:51Z

pkg/cmd/server/origin/controller.go

+		HasStatefulSetsEnabled: c.Options.DisabledFeatures.Has("triggers.image.openshift.io/statefulsets"),
+		HasCronJobsEnabled:     c.Options.DisabledFeatures.Has("triggers.image.openshift.io/cronjobs"),
+	}
+	ret["image-trigger"] = imageTrigger.RunController


openshift.io/*

deads2k · 2017-06-06T18:13:57Z

pkg/cmd/server/origin/controller.go

+		DisableScheduledImport:                     c.Options.ImagePolicyConfig.DisableScheduledImport,
+		ScheduledImageImportMinimumIntervalSeconds: c.Options.ImagePolicyConfig.ScheduledImageImportMinimumIntervalSeconds,
+	}
+	ret["image-import"] = imageImport.RunController


openshift.io/*

deads2k · 2017-06-06T18:14:02Z

pkg/cmd/server/origin/controller.go

+			CertFile: c.Options.ControllerConfig.ServiceServingCert.Signer.CertFile,
+			KeyFile:  c.Options.ControllerConfig.ServiceServingCert.Signer.KeyFile,
+		}
+		ret["service-serving-cert"] = serviceServingCert.RunController


openshift.io/*

deads2k · 2017-06-06T18:14:35Z

I'd like to see #14333 merge first. Can you rebase on top of it?

deads2k · 2017-06-06T18:15:43Z

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

+	}
+	if !c.HasDeploymentsEnabled {
+		sources = append(sources, imagetriggercontroller.TriggerSource{
+			Resource:  schema.GroupResource{Group: "extensions", Resource: "deployments"},


open an issue to update this to apps or we're going to have a problem.

deads2k · 2017-06-06T18:17:17Z

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

+	case *kappsv1beta1.StatefulSet:
+		_, err := u.kclient.Apps().StatefulSets(t.Namespace).Update(t)
+		return err
+	case *kapiv1.Pod:


we're touching pods for some reason?

@smarterclayton ^

deads2k · 2017-06-06T18:18:35Z

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

+
+	go controller.Run(5, ctx.Stop)
+	if c.DisableScheduledImport {
+		return true, nil


move to the beginning of the function. return false, nil

deads2k · 2017-06-06T18:20:00Z

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

+	KeyFile  string
+}
+
+func (c *ServiceServingCertsControllerOptions) RunController(ctx ControllerContext) (bool, error) {


if there's no cert and no key, return false, nil

deads2k · 2017-06-06T18:20:12Z

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

+func (c *ServiceServingCertsControllerOptions) RunController(ctx ControllerContext) (bool, error) {
+	ca, err := crypto.GetCA(c.CertFile, c.KeyFile, "")
+	if err != nil {
+		return false, fmt.Errorf("service serving cert controller: %v", err)


return true, fmt.Errorf...

mfojtik · 2017-06-12T12:19:15Z

@deads2k or @smarterclayton can you please review the start_master and the image controller file? thats where the most conflict were, I hope i fixed them properly and don't miss anything.

deads2k · 2017-06-12T13:25:51Z

@deads2k or @smarterclayton can you please review the start_master and the image controller file? thats where the most conflict were, I hope i fixed them properly and don't miss anything.

still lgtm

mfojtik · 2017-06-12T14:19:54Z

@enj

mfojtik · 2017-06-13T08:49:16Z

[merge][severity: blocker]

mfojtik · 2017-06-13T11:32:08Z

@deads2k @smarterclayton now that i fixed the build integration test to incorporate the informers and rebased this, i'm back in position 10... i'm trying to merge this for ~8 days now and it is becoming hell to rebase this and get back to the beggining on the queue every morning...

can we green button merge this? i'm willing to rebase this once again after generated auth informers go in, but if we want this in 3.6, it would be better to green button merge it.

deads2k · 2017-06-13T11:54:22Z

@deads2k @smarterclayton now that i fixed the build integration test to incorporate the informers and rebased this, i'm back in position 10... i'm trying to merge this for ~8 days now and it is becoming hell to rebase this and get back to the beggining on the queue every morning...

can we green button merge this? i'm willing to rebase this once again after generated auth informers go in, but if we want this in 3.6, it would be better to green button merge it.

@smarterclayton @eparis @pweil- this has happened to multiple pulls over the past two sprints and is symptomatic of our larger queue problems. While the green button provides us with some degree of control, manual queue management like this is unsustainable in the long run and inherently favoritist.

mfojtik · 2017-06-13T12:14:17Z

@jupierce ^

mfojtik · 2017-06-14T10:44:20Z

@smarterclayton @eparis @jupierce another rebase, another queue bump: You are in the build queue at position: 18 :-( (to be clear, all I did was rebase and fix 1 conflict which teleports this from position 9 to position 18... not mentioning that this PR was at position 10 yesterday evening?)

That means this won't get into DCUT (by normal merge) and since we really need this for 3.6 i'm going to claim this as a blocker and keep merging this during DCUT.

It also opens a question how much effictive is the DCUT merge close. If a developer working on a feature (big enough to caught multiple conflicts during merge process) is not able to merge his feature during the 1.5w period AND he started merging the feature at day 1 when the sprint begins, I think that is something really concerning and we should back off and reevaluate our merge process.

/me sorry for the rant, but this is really frustrating.

enj · 2017-06-14T23:54:30Z

[severity:blocker]

mfojtik · 2017-06-15T07:32:58Z

@enj ❤️

mfojtik · 2017-06-15T07:34:45Z

broken by #14289 ...

deads2k · 2017-06-15T11:44:59Z

[severity:blocker]

Agree

…controllers

openshift-bot · 2017-06-15T14:07:01Z

Evaluated for origin test up to 97efc6c

smarterclayton · 2017-06-15T15:59:11Z

Compile error

openshift-bot · 2017-06-15T16:06:12Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2267/) (Base Commit: c323526)

mfojtik changed the title ~~Refactor controller initialization (part 3)~~ WIP: Refactor controller initialization (part 3) May 24, 2017

mfojtik force-pushed the controller-init-3 branch from a394e93 to 654779a Compare May 24, 2017 11:07

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

mfojtik mentioned this pull request Jun 5, 2017

Sync images from one image stream to another registry #14471

Merged

mfojtik force-pushed the controller-init-3 branch from 45fa238 to 85d12ed Compare June 5, 2017 18:18

mfojtik changed the title ~~WIP: Refactor controller initialization (part 3)~~ Refactor controller initialization (round #3) Jun 5, 2017

mfojtik requested review from smarterclayton and soltysh June 5, 2017 18:25

mfojtik force-pushed the controller-init-3 branch from 85d12ed to 2c55a39 Compare June 5, 2017 18:27

mfojtik commented Jun 5, 2017

View reviewed changes

mfojtik force-pushed the controller-init-3 branch from 2c55a39 to 25d773b Compare June 5, 2017 18:31

mfojtik commented Jun 5, 2017

View reviewed changes

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

soltysh approved these changes Jun 6, 2017

View reviewed changes

mfojtik force-pushed the controller-init-3 branch from 25d773b to de25c2e Compare June 6, 2017 09:44

deads2k reviewed Jun 6, 2017

View reviewed changes

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 11, 2017

mfojtik force-pushed the controller-init-3 branch from 23b7b5a to 3c6406a Compare June 12, 2017 12:17

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

mfojtik force-pushed the controller-init-3 branch from 8c3189f to bfb13e6 Compare June 14, 2017 08:23

mfojtik mentioned this pull request Jun 14, 2017

Refactor controller initialization (final round?) #14633

Merged

deads2k mentioned this pull request Jun 14, 2017

simplify controller wiring #14333

Closed

mfojtik force-pushed the controller-init-3 branch from bfb13e6 to 45b7d86 Compare June 15, 2017 07:47

mfojtik force-pushed the controller-init-3 branch from 45b7d86 to 016d4b9 Compare June 15, 2017 11:50

deads2k and others added 3 commits June 15, 2017 15:53

simplify controller wiring

e3c1b6e

refactor initialization for image, service serving certs and trigger …

f41aa48

…controllers

fix buildcontroller integration test to use generated informers

97efc6c

mfojtik force-pushed the controller-init-3 branch from 016d4b9 to 97efc6c Compare June 15, 2017 13:53

smarterclayton merged commit 84b8802 into openshift:master Jun 15, 2017

mfojtik deleted the controller-init-3 branch September 5, 2018 21:07

Refactor controller initialization (round #3) #14317

Refactor controller initialization (round #3) #14317

Conversation

mfojtik commented May 24, 2017 • edited Loading

mfojtik commented May 24, 2017

mfojtik commented May 24, 2017

mfojtik commented Jun 5, 2017

mfojtik commented Jun 5, 2017

mfojtik commented Jun 5, 2017

mfojtik commented Jun 5, 2017

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

soltysh 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

deads2k commented Jun 6, 2017

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

mfojtik commented Jun 12, 2017

deads2k commented Jun 12, 2017

mfojtik commented Jun 12, 2017

mfojtik commented Jun 13, 2017 • edited by eparis Loading

mfojtik commented Jun 13, 2017

deads2k commented Jun 13, 2017

mfojtik commented Jun 13, 2017

mfojtik commented Jun 14, 2017 • edited Loading

enj commented Jun 14, 2017 • edited by jupierce Loading

mfojtik commented Jun 15, 2017

mfojtik commented Jun 15, 2017

deads2k commented Jun 15, 2017

openshift-bot commented Jun 15, 2017

smarterclayton commented Jun 15, 2017

openshift-bot commented Jun 15, 2017

mfojtik commented May 24, 2017 •

edited

Loading

mfojtik commented Jun 13, 2017 •

edited by eparis

Loading

mfojtik commented Jun 14, 2017 •

edited

Loading

enj commented Jun 14, 2017 •

edited by jupierce

Loading